InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.64k stars 902 forks source link

SimpleWeather service : new weather implementation #1924

Closed JF002 closed 6 months ago

JF002 commented 6 months ago

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.

image

image

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::optional fields. std::optional is 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?

github-actions[bot] commented 6 months ago
Build size and comparison to main: Section Size Difference
text 368984B -8440B
data 940B 0B
bss 63516B 56B
FintasticMan commented 6 months ago

We should also remove the old weather debug app, but that can be done in another PR.

JF002 commented 6 months ago

We should also remove the old weather debug app, but that can be done in another PR.

Thanks for the review! :+1:

I think I addressed all the points you mentioned !

JF002 commented 6 months ago

@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). I don't have a strong opinion about the precision of the temperature. If you think we need more precision the 1 degree, we can indeed use int16 instead of int8.

FintasticMan commented 6 months ago

Yeah, I think it is best if we use int16 for the temperature, as it allows a lot more freedom. The Fahrenheit value would also be off by 1 or 2 degrees if we only use 1 degree of Celsius precision.

JF002 commented 6 months ago

@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.