GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — SimpleWeather service : new weather implementation by JF002 · Pull Request #1924 · InfiniTimeOrg/InfiniTime

Conversation

@JF002

@JF002 JF002 commented Dec 9, 2023

Copy link
Copy Markdown
Collaborator

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:

  • The memory usage of the WeatherService is virtually infinite : older events are erased only when they expires. The memory usage grows if the companion app sends new data while the previous ones are still valid.
  • The API does not provide any way for the companion apps to help manage the memory usage (they do not know what events were already sent by this or another companion app).
  • Since multiple events of the same type can be stored in the timeline, InfiniTime does not know which value must be displayed as "current temperature", for example.
  • The timeline implementation allows a lot of advanced use-cases, but is, IMHO, too complex to support simple use-cases like the one we implement right now (display the current weather on the watch face).

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jmlich, Elara6331, NeroBurner, minacode, and dav23r reacted with heart emoji
All reactions
  • ❤️ 5 reactions
@github-actions

github-actions Bot commented Dec 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 368984B -8440B
data 940B 0B
bss 63516B 56B
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jmlich jmlich mentioned this pull request Dec 12, 2023
Infinitime Simple Weather Service piggz/harbour-amazfish#319
Merged
FintasticMan
FintasticMan requested changes Dec 15, 2023
View reviewed changes

@FintasticMan FintasticMan left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/SimpleWeatherService.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread doc/SimpleWeatherService.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/displayapp/screens/StopWatch.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/displayapp/screens/WatchFacePineTimeStyle.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan FintasticMan added the weather Bugs and PRs related to Weather label Dec 15, 2023
@FintasticMan

FintasticMan commented Dec 17, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002

JF002 commented Dec 18, 2023

Copy link
Copy Markdown
Collaborator Author

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 !

FintasticMan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan requested changes Dec 18, 2023
View reviewed changes
Comment thread src/displayapp/screens/Symbols.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan approved these changes Dec 19, 2023
View reviewed changes
@JF002 JF002 added this to the 1.14.0 milestone Dec 21, 2023
@JF002 JF002 mentioned this pull request Dec 21, 2023
PTS: Add setting to display temperature in Fahrenheit #1805
Merged
JF002 added 6 commits December 21, 2023 21:08
@JF002
SimpleWeather service : new weather implementation
a865886
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.
@JF002
SimpleWeather service : new weather implementation
4e75d64
Fix recovery firmware and code formatting.
@JF002
Simple Weather Service : fix out of bounds access while creating Fore…
a3b6cdb
…cast instance.
@JF002
Simple Weather Service - code cleaning and improvements
f2decc7
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.
@JF002
Simple Weather Service - code cleaning and improvements
1daecd2
Remove unused Weather debug app.
Fix formatting in SimpleWeatherService.cpp.
@JF002
Simple Weather Service - code cleaning and improvements
e9ebdd7
Rename Symbols::cloud_meatball to Symbols::cloudMeatball.
@JF002
JF002 force-pushed the simple-weather-service branch from edf480b to e9ebdd7 Compare December 21, 2023 20:09
@JF002 JF002 mentioned this pull request Dec 21, 2023
Integration of Simple Weather Service InfiniTimeOrg/InfiniSim#135
Merged
FintasticMan
FintasticMan requested changes Dec 21, 2023
View reviewed changes

@FintasticMan FintasticMan left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@JF002

JF002 commented Dec 21, 2023

Copy link
Copy Markdown
Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Dec 21, 2023

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan requested changes Dec 21, 2023
View reviewed changes

@FintasticMan FintasticMan left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/components/ble/SimpleWeatherService.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/displayapp/screens/WatchFacePineTimeStyle.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

JF002 added 3 commits December 23, 2023 15:54
@JF002
Simple Weather Service
36c8032
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).
@JF002
Simple Weather Service
84d7985
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.
@JF002
Simple Weather Service
d6eb61e
Store temperatures as int16_t (instead of uint8_t previously). The temperature is expressed in °C * 100.
@JF002

JF002 commented Dec 23, 2023

Copy link
Copy Markdown
Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002
Simple Weather Service
feddb82
Fix code formatting.
NeroBurner
NeroBurner requested changes Dec 23, 2023
View reviewed changes
Comment thread doc/ble.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@JF002
Simple Weather Service
b28ec60
Fix typo in doc/ble.md.
FintasticMan
FintasticMan approved these changes Dec 23, 2023
View reviewed changes

@FintasticMan FintasticMan left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/components/ble/SimpleWeatherService.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

NeroBurner
NeroBurner approved these changes Dec 23, 2023
View reviewed changes

@NeroBurner NeroBurner left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread 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

@NeroBurner NeroBurner Dec 23, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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"

Suggested change
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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@JF002 JF002 Dec 23, 2023

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/components/ble/SimpleWeatherService.cpp Outdated

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);

@NeroBurner NeroBurner Dec 23, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@JF002 JF002 Dec 23, 2023

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oof good catch! Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@JF002
Simple Weather Service
4c7569f
Fix ToUInt64() in SimpleWeatherService.cpp.
Fix typo in SimpleWeatherService.md.
@JF002
JF002 merged commit ca7d8a6 into main Dec 23, 2023
@JF002
JF002 deleted the simple-weather-service branch December 23, 2023 20:12
This was referenced Dec 23, 2023
Improve weather service #1822
Closed
Improve weather service #1890
Closed
weather: Add support for conditions #1904
Closed
RFC: Timeline event packing #1878
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FintasticMan FintasticMan FintasticMan approved these changes
@NeroBurner NeroBurner NeroBurner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

weather Bugs and PRs related to Weather

Projects

None yet

Milestone

1.14.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@JF002 @FintasticMan @NeroBurner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.