Skip to content

Conversation

@nconner-06
Copy link

Move the variables for LAT and LOG to float for OpenWeathermap to floats

@nconner-06 nconner-06 changed the title Dev weather - linked to issue #287 Dev weather - linked to issue #287 / #298 / #288 plus additional feed Mar 16, 2025
@nconner-06
Copy link
Author

This PR handles the following issues:

And includes a new feed from Pirate Weather

dreed47 and others added 3 commits March 16, 2025 21:55
…agement

Consolidate Widget draw and update frequency management
Switch to new API provider for the timeZone information
Copy link
Collaborator

@flattermann flattermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also comment out the WEATHER_* #defines in config.h.template so that the user can enable what he needs.

And move the default values to config.system.h (if #ifndef) because we might otherwise get compilation errors.

#elif WEATHER_VISUALCROSSING_FEED
return new VisualCrossingFeed(WEATHER_VISUALCROSSING_API_KEY, weatherUnits);
#endif
switch (WEATHER_FEED) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure, but I think a switch might cause some problems here, because it might compile/link the unnecessary feeds as well? Needs testing.

We could switch to #if WEATHER_FEED == VISUALCROSSING if necessary (but we need to #define VISUALCROSSING 0 because enum items can't be used by the preprocessor)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback, I'll look at your comments.

Did you look at the weather feed .h file, each has a #ifndef section to define the variables, thus I don't know if we need it in config.system.h as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the header files for all the feeds are included in WeatherWidget.cpp, it looks like all the feeds get compiled and linked - even if I change to a "#if" condition...
image

Copy link
Collaborator

@flattermann flattermann Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. If the defaults are already set in the individual feed .h, we don't need them in config.system.h.

Maybe include the feeds conditionally as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested both way - switch and ifdef (including ifdef on the fee.h files) and it appears that all feeds are included in the compile / ink.. not sure if I was doing any incorrect, if you know a way, then I'm open to suggestions.

@nconner-06 nconner-06 requested a review from flattermann March 18, 2025 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants