GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — SimpleWeather service : new weather implementation by JF002 · Pull Request #1924 · InfiniTimeOrg/InfiniTime Conversation
Collaborator
IntroductionThe 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 APIThe 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. Implementation and integration in InfiniTimeThe 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 I integrated the SimpleWeatherService in PTS to provide the same functionality than before. What's nextI'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?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
|
Build size and comparison to main:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think this is quite a reasonable API given our current memory limitations and issues we've had with the current weather service.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
We should also remove the old weather debug app, but that can be done in another PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Thanks for the review! 👍 I think I addressed all the points you mentioned ! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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.
edf480b to
e9ebdd7
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yeah, I think it is best if we use |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A couple more things I noticed going through the code again :-)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Fix code formatting.
There was an error while loading. Please reload this page.
Fix typo in doc/ble.md.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
format and a small bug in uint64 conversion, afterwards all good (at least the parts I understand about the Bluetooth stack)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| ## 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.
The reason will be displayed to describe this comment to others. Learn more.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| 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.
The reason will be displayed to describe this comment to others. Learn more.
byte idx 5 and byte idx 6 are both shifted by 48 bits
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oof good catch! Thanks!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Fix ToUInt64() in SimpleWeatherService.cpp. Fix typo in SimpleWeatherService.md.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.