Improve weather service #1890
Improve weather service #1890
Conversation
Co-authored-by: FintasticMan <[email protected]>
Before further parsing of the packet, check timestamp and expires field right after it has been decoded to avoid unnecessary allocs/frees. Also perform tidying up of the vector before attempting to add any new elements.
This means that there is a higher likelihood that the returned event is the most up-to-date current event. It chooses the event with the lowest expiry, and between events with the same expiry, it chooses the one with the latest timestamp.
This allows us to use new functionalities like "if constexpr" and std::optional.
|
Build size and comparison to main:
|
| /** | ||
| * Unique ID for this event | ||
| */ | ||
| uint16_t eventID; |
There was a problem hiding this comment.
I'm more and more coming to the conclusion that this would in practice behave as a "mutability flag" of sorts.
So could we maybe instead of an ID use a boolean "overwrite this with the ones coming in with the same flag"? We can keep it an uint8_t and use a single bit for starters?
Meaning currently companions will probably agree that e.g. 1 will be next upcoming temperature, 2 will be next upcoming precipitation and so on. But this makes it hard for more advanced companions to push say "next week" without interfering with other companions. (Though I don't know if this should really be our problem. Is it with BLE FS, Music or notifications?)
This way we could avoid the previously mentioned caveats by just marking events "replaceable" and it provides one "immediate" slot for each event, kinda.
There was a problem hiding this comment.
This flag could also be set a default and we could handle it being missing from the CBOR packet. This way we can also avoid a breaking API change.
| int64_t tmpAltitude = 0; | ||
| QCBORDecode_GetInt64InMapSZ(&decodeContext, "Altitude", &tmpAltitude); | ||
| if (tmpAltitude < -32768 || tmpAltitude >= 32767) { | ||
| auto optAltitude = Get<int16_t>(&decodeContext, "Altitude"); |
There was a problem hiding this comment.
Fun fact, this data could be used in the TImer app to adjust the time it takes to boil water for eggs for example.
Add a sane max number of elements in the timeline Create events in their own factory methods (CreateXXXEvent()) Encapsulate QCBOR deserialization in template functions Get<>(). Co-authored-by: FintasticMan <[email protected]>
Make sure there is only 1 of each event with the same type and ID. This is a BREAKING CHANGE for the WeatherService API, and as such a new EventID field will need to be added to the CBOR data sent by companion apps.
FintasticMan
force-pushed
the
fix_weather
branch
from
01286aa to
4cf494e
Compare
|
Thanks to everyone involved in this PR. I'm closing it in favor to #1924 since I decided to rework the weather service so we can fix the issues more easily. |
This pull request adds a number of changes to WeatherService and WeatherData that improve its functionality and stability. These changes are partially based on @faxe1008's work in #1822, @JF002's work in #1860 and my work in #1882.
The change I made to #1822 is to not use a template function for the general GetCurrent function, but rather let the GetCurrent* functions do the cast themselves. This reduces the number of functions created in total.
One of the big improvements is the addition of an event ID. This ID will allow companion apps to update values they have already sent with new information, allowing them to operate statelessly.
This is a BREAKING CHANGE to the WeatherService API, meaning that companion apps will need to update to add an EventID field to the CBOR data for every event that they send. If this PR gets approved, I will make prepare patches for all the companion apps that implement WeatherService to make the change smoother.
I propose that we create a list of standardised IDs for events that most companion apps will implement, which will allow the user to switch companion apps and have the weather data update seamlessly.
Companion apps will then also be able to create their own IDs outside of the standard ones that won't be affected by other companions.
One change I would still like to make is to move away from using std::strings and switch to using std::array, to reduce the uncertainty of how long the string might be.
@JF002 @Avamander @faxe1008 could you please give some feedback on these changes.
Fixes #1786, #1788 and #1877.