Should the code of the openweathermap binding be repointed to its own location provider?

teachrockstothink · November 21, 2024, 11:53am

I went to configure the OpenWeatherMap binding. I put in the LatLong location in the Location field for the Binding in preferences, but still got the error:

2024-11-19 22:20:14.271 [DEBUG] [overy.OpenWeatherMapDiscoveryService] - Location is not set → Will not provide any discovery results.

So then I went and manually edited the file

/etc/openhab/services/runtime.cfg

To put in the Lat and Long under the entry:

org.openhab.i18n:location=

Then it started working.

That to me suggests that the GUI field isn’t wired up.

When I look at the code here:

github.com

openhab/openhab-addons/blob/main/bundles/org.openhab.binding.openweathermap/src/main/java/org/openhab/binding/openweathermap/internal/discovery/OpenWeatherMapDiscoveryService.java#L62


      
          
          private final OpenWeatherMapAPIHandler bridgeHandler;
          
          /**
           * Creates an OpenWeatherMapLocationDiscoveryService.
           */
          public OpenWeatherMapDiscoveryService(OpenWeatherMapAPIHandler bridgeHandler, LocationProvider locationProvider,
                  LocaleProvider localeProvider, TranslationProvider i18nProvider) {
              super(AbstractOpenWeatherMapHandler.SUPPORTED_THING_TYPES, DISCOVERY_TIMEOUT_SECONDS);
              this.bridgeHandler = bridgeHandler;
              this.locationProvider = locationProvider;
              this.localeProvider = localeProvider;
              this.i18nProvider = i18nProvider;
              activate(null);
          }
          
          @Override
          protected void activate(@Nullable Map<String, Object> configProperties) {
              super.activate(configProperties);
          }

It uses the discovery process from the centralised configuration:

import org.openhab.core.i18n.LocationProvider;

Not its own local configuration here:

github.com

openhab/openhab-addons/blob/main/bundles/org.openhab.binding.openweathermap/src/main/java/org/openhab/binding/openweathermap/internal/config/OpenWeatherMapLocationConfiguration.java#L26


      
          
          /**
           * The {@link OpenWeatherMapLocationConfiguration} is the class used to match the
           * {@link org.openhab.binding.openweathermap.internal.handler.AbstractOpenWeatherMapHandler}s
           * configuration.
           *
           * @author Christoph Weitkamp - Initial contribution
           */
          @NonNullByDefault
          public class OpenWeatherMapLocationConfiguration {
              public @NonNullByDefault({}) String location;
          }

So to compare it to Astro - which does use local location:

github.com

openhab/openhab-addons/blob/91f16fa16c340910282e1a116291453c365eeea7/bundles/org.openhab.binding.astro/src/main/java/org/openhab/binding/astro/internal/config/AstroThingConfig.java#L28


      
          /**
           * Astro Thing configuration.
           *
           * @author Gerhard Riegler - Initial contribution
           */
          @NonNullByDefault
          public class AstroThingConfig {
              public static final String GEOLOCATION = "geolocation";
              public static final String USE_METEOROLOGICAL_SEASON = "useMeteorologicalSeason";
              public @Nullable String geolocation;
              public @Nullable Double altitude;
              public @Nullable Double latitude;
              public @Nullable Double longitude;
              public boolean useMeteorologicalSeason;
              public int interval = 300;
          
              /**
               * Splits the geolocation into latitude and longitude.
               */
              public void parseGeoLocation() {
                  if (geolocation != null) {

and read the configuration.

github.com

openhab/openhab-addons/blob/91f16fa16c340910282e1a116291453c365eeea7/bundles/org.openhab.binding.astro/src/main/java/org/openhab/binding/astro/internal/discovery/AstroDiscoveryService.java#L73


      
          private @Nullable PointType previousLocation;
          
          @Activate
          public AstroDiscoveryService(final @Reference LocationProvider locationProvider,
                  final @Reference LocaleProvider localeProvider, final @Reference TranslationProvider i18nProvider,
                  @Nullable Map<String, Object> configProperties) {
              super(new HashSet<>(Arrays.asList(new ThingTypeUID(BINDING_ID, "-"))), DISCOVER_TIMEOUT_SECONDS, true);
              this.locationProvider = locationProvider;
              this.localeProvider = localeProvider;
              this.i18nProvider = i18nProvider;
              activate(configProperties);
          }
          
          @Override
          protected void startScan() {
              logger.debug("Starting Astro discovery scan");
              PointType location = locationProvider.getLocation();
              if (location == null) {
                  logger.debug("LocationProvider.getLocation() is not set -> Will not provide any discovery results");
                  return;
              }

My question is: Does it seem reasonable to copy the code from Astro in a PR for OpenWeatherMaps to make it load location from the GUI instead of the file?

rlkoshak · November 21, 2024, 2:40pm

I don’t know the answer to your question but I moved this post to the development category where it’s more likely to get the attention of those who can.

I do not know the overall impacts of making this proposed change but it seems reasonable.

hmerk · November 21, 2024, 3:00pm

Yes, please provide a PR and let our maintainers review it.