SimpleWeather service : new weather implementation #1924
Conversation
|
Build size and comparison to main:
|
|
We should also remove the old weather debug app, but that can be done in another PR. |
Thanks for the review! 👍 I think I addressed all the points you mentioned ! |
This new implementation of the weather feature provides a new BLE API and a new weather service. The API uses a single characteristic that allows companion apps to write the weather conditions (current and forecast for the next 5 days). The SimpleWeather service exposes those data as std::optional fields. This new implementation replaces the previous WeahterService. The API is documented in docs/SimpleWeatherService.md.
Fix recovery firmware and code formatting.
Add missing icons (heavy clouds, thunderstorm, snow). Remove unneeded comparison operator (!=), improve conversion of Timestamp and MessageType, order includes. Fix typo in documentation. Remove not related change in StopWatch.
Remove unused Weather debug app. Fix formatting in SimpleWeatherService.cpp.
Rename Symbols::cloud_meatball to Symbols::cloudMeatball.
JF002
force-pushed
the
simple-weather-service
branch
from
edf480b to
e9ebdd7
Compare
There was a problem hiding this comment.
As I was looking to implement #1805 for this new service, I noticed that the temperatures are stored as unsigned integers, which doesn't allow for negative values! This is definitely an issue, because temperatures below 0 are very common.
I'm also wondering if we might want more precision than 1 degree. The way the previous weather service implemented it is that the temperatures were stored in int16s in degrees Celcius multiplied by 100. (This would also solve the issue where we aren't able to store temps above 127 degrees, but I don't think this is really an issue.)
|
@FintasticMan You are right, temperatures should be stored as signed values! I'll fix this! I figured that values from -128 to +127 was indeed a reasonable range (that's also a reason why the temperature are expressed in °C in the BLE protocol). |
|
Yeah, I think it is best if we use |
Code improvements : icon fields are now typed as Icons, move the location string when creating a new instance of CurrentWeather, fix SimpleWeatherService::CurrentWeather::operator== (location was missing from the comparison).
Move the function GetIcon that converts SimpleWeatherService::Icons to char (symbol) into a new header file so that it can be used by other apps and companion apps.
Store temperatures as int16_t (instead of uint8_t previously). The temperature is expressed in °C * 100.
|
@FintasticMan Thanks again for this very deep review :) I updated the protocol so that it uses int16_t instead of uint8_t to store the temperature. I'll also update the PR in Amazfish to reflect those changes. |
Fix code formatting.
Fix typo in doc/ble.md.
doc/SimpleWeatherService.md
Outdated
|
|
||
| ## Introduction | ||
|
|
||
| The Simple Weather Service provide a simple and straightforward API to specify the current weather and the forecast for the next 5 days. It effectively replaces the original Weather Service (from InfiniTime 1.8) since InfiniTime 1.14 |
There was a problem hiding this comment.
one line per sentence for easier diffs in the future + "The .. Service provides"
| The Simple Weather Service provide a simple and straightforward API to specify the current weather and the forecast for the next 5 days. It effectively replaces the original Weather Service (from InfiniTime 1.8) since InfiniTime 1.14 | |
| The Simple Weather Service provides a simple and straightforward API to specify the current weather and the forecast for the next 5 days. | |
| It effectively replaces the original Weather Service (from InfiniTime 1.8) since InfiniTime 1.14. |
|
|
||
| uint64_t ToUInt64(const uint8_t* data) { | ||
| return data[0] + (data[1] << 8) + (data[2] << 16) + (data[3] << 24) + (static_cast<uint64_t>(data[4]) << 32) + | ||
| (static_cast<uint64_t>(data[5]) << 48) + (static_cast<uint64_t>(data[6]) << 48) + (static_cast<uint64_t>(data[7]) << 56); |
There was a problem hiding this comment.
byte idx 5 and byte idx 6 are both shifted by 48 bits
There was a problem hiding this comment.
Oof good catch! Thanks!
Fix ToUInt64() in SimpleWeatherService.cpp. Fix typo in SimpleWeatherService.md.
Introduction
The Weather Service was released in InfiniTime 1.8 and, while it has been integrated by multiple companion apps (like ITD, Amazfish, Gadgetbridge,...), we only started using it with InfiniTime 1.13, when we implemented the weather display in the PTS watchface.
Unfortunately, it didn't take long before we received reports of incorrect values being displayed, strange font corruption and crashes.
Multiple contributors analyzed these issues and tried to fix them (here, here, here, here, here, and here) but unfortunately, we weren't able to find a satisfying solution to those issues.
So I decided to have a closer look at those bug reports to try to understand what was happening. I wrote my observations here. Here is a summary:
Like a few other contributors, I talked to developers who have already worked on the weather feature, I did many tests and experiments, I tried to find solutions to improve the current implementation, but in the end, I couldn't find a satisfying solution to make the weather feature reliable, stable and easy to maintain.
I eventually came to the conclusion that the current implementation and API do not fit our needs, and that we will have to start designing the feature from scratch. It was not an easy decision to take : I know that many people spent a lot of their precious time working on the current implementation and on the integration of the API in companion apps. Starting from scratch seemed like a huge waste of time and energy but... I couldn't find any other solution.
To design this new implementation of the weather feature, I first listed the use-cases we want to support : display the current weather (temperature and weather condition like sun, rain, clouds,...) and forecast for the next few days.
I then talk with a few companion apps developers (Adam and Josef from Amazfish, Elara from ITD) so that I could design a BLE API that allows companion apps to provide the data needed by InfiniTime. Josef even provided me with a branch in Amazfish that implements the new weather protocol so that I could implement and test it in InfiniTime.
And finally, I implemented the API in InfiniTime and ensure that the data are correctly displayed in PTS.
BLE API
The BLE API is documented in doc/SimpleWeatherService.md. It's a very simple service that provide a single characteristic. The companion app uses this characteristic to write the current weather information and the forecast for the next 5 days.
Since the API uses a message type field and a version field, it can be extended in the future if we need to add more data to the weather information or implement new uses-cases.
Implementation and integration in InfiniTime
The implementation (in src/components/ble/SimpleWeatherService.h and src/components/ble/SimpleWeatherService.cpp) of the API in InfiniTime first parses and validates the data from the BLE characteristic and then store those data. It does not store multiple versions of the data though : the new data always override the previous ones. SimpleWeatherService exposes those data as
std::optionalfields.std::optionalis a tool from C++17 that manage a value that may or may not be present. This comes in handy to express that weather were received or not and if they are still valid or not.I integrated the SimpleWeatherService in PTS to provide the same functionality than before.
What's next
I'll leave this PR open for review for a few days to let develops provide feedback about it. I'm mostly looking for feedback from companion apps developers : what do you think of the API? Do you plan on integrating it in a future release of your app?