GeezerVison

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

GeezerVison is showing github.com — weather: Add function for converting to units by FintasticMan · Pull Request #2015 · InfiniTimeOrg/InfiniTime

Conversation

FintasticMan commented Feb 12, 2024

Copy link

Copy Markdown

Member

Handles rounding correctly.

Sorry, something went wrong.

Uh oh!

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

All reactions

FintasticMan mentioned this pull request

Feb 12, 2024

weather: Add new app with forecast #1995

Merged

github-actions Bot commented Feb 12, 2024 •

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	374576B	48B
data	948B	0B
bss	63504B	16B

All reactions

Sorry, something went wrong.

Uh oh!

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

vkareh reviewed

Feb 12, 2024

View reviewed changes

Comment thread

src/components/ble/SimpleWeatherService.h Resolved

Uh oh!

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

FintasticMan force-pushed the temp_to_units branch from d8a0959 to f07699a Compare

February 15, 2024 11:10

minacode commented Feb 15, 2024

Copy link

Copy Markdown

Contributor

You could use structs of one int to make the types really safe.

All reactions

Sorry, something went wrong.

Uh oh!

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

JF002 commented Feb 15, 2024

Copy link

Copy Markdown

Collaborator

I'm ok with the current state. I however thought about another option.

SimpleWeatherService would define a Temperature type : PineTime::Controllers::SimpleWeatherService::Temperature. It's the internal representation of a temperature in the SompleWeatherService expressed in "°C * 100"

struct Temperature {
  int16_t temperature;
}

Somewhere under DisplayApp, we would define another Temperature type : Pinetime::Applications::Temperature. It represent the temperature for the UI side of InfiniTime as an std::variant (TODO : check that the overhead of std::variant is negligible)

struct TemperatureCelsius {
  int16_t temperature;
}

struct TemperatureFahrenheit {
  int16_t temperature;
}

using Temperature = std::variant<TemperatureCelsius, TemperatureFahrenheit temperature

Then, also under DisplayApp, we define a function that converts from SimpleWeatherService Temperature to "display" temperature :

Pinetime::Applications::Temperature Convert(PineTime::Controllers::SimpleWeatherService::Temperature, Controllers::Settings::WeatherFormat format);

This way, all the "temperature" types are strongly typed and it's impossible to implicitly convert from one to another. Also SimpleWeatherService does not have any code related to the UI anymore.

minacode and FintasticMan reacted with rocket emoji

All reactions

🚀 2 reactions

Sorry, something went wrong.

Uh oh!

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

NeroBurner added the maintenance Background work label

Oct 1, 2024


          weather: Refactor temperature type for type safety

be5a916

There is now a Temperature struct in the weather service, which holds
the internal representation. There is also a temperature struct in the
Applications namespace, which holds the temperature in either Celsius or
Fahrenheit.

FintasticMan force-pushed the temp_to_units branch from f07699a to b9fd12e Compare

October 2, 2024 09:59


          Switch to simpler temperature interface

FintasticMan force-pushed the temp_to_units branch from b9fd12e to 2462696 Compare

October 2, 2024 10:06

FintasticMan commented

Oct 2, 2024

View reviewed changes

FintasticMan left a comment

Copy link

Copy Markdown

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

@JF002 I've just refactored this a bit, what do you think? I wasn't able to use std::variant, as it is very strict about knowing which variant to use at compile-time. That meant that we needed 2 completely separate code paths for Celsius and Fahrenheit.

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 Outdated


		class Temperature {
		public:
		explicit Temperature(int16_t raw = 0) : raw {raw} {

FintasticMan Oct 2, 2024

Copy link

Copy Markdown

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

I'm not a big fan of having the default value be 0 here, but otherwise the compiler complains about there being no default value for the array of Days in the Forecast.

Sorry, something went wrong.

Uh oh!

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

All reactions

minacode Oct 2, 2024

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.

I think it would be conceptually better (maybe not memory-wise) to make the days array of type:
std::array<std::optional<Day>, nbOfDays>. Then you would not need to construct Days without values.

Sorry, something went wrong.

Uh oh!

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

All reactions

L3br4nd Oct 8, 2024

Copy link

Copy Markdown

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.

Would defining Temperature in Kelvin internally and converting to DisplayTemperature in °C or °F depending on the user setting for displaying be helpful?
Sorry, I'm not into object based programming that much...

Sorry, something went wrong.

Uh oh!

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

All reactions

minacode Oct 8, 2024

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.

I don't think that is the problem here. The internal representation is never shown directly and can therefore be whatever. The problem is, that right now we need a temperature value in a place where no value should be.

Sorry, something went wrong.

Uh oh!

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

All reactions

FintasticMan Oct 8, 2024

Copy link

Copy Markdown

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

I think std::optional does make sense, I'm going to see how implementing it like that looks.

Sorry, something went wrong.

Uh oh!

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

minacode reacted with thumbs up emoji

All reactions

👍 1 reaction

FintasticMan Oct 9, 2024

Copy link

Copy Markdown

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

I think this works quite well, what do you think?

Sorry, something went wrong.

Uh oh!

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

All reactions

NeroBurner added this to the 1.15.0 milestone

Oct 2, 2024

minacode commented Oct 2, 2024

Copy link

Copy Markdown

Contributor

I like the current approach with the raw value and the two representations! :)

FintasticMan reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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


          weather: Switch to std::optional for Forecast days

f30d778

Also only iterate over the number of days actually in use, rather than
MaxNbForecastDays.

FintasticMan commented

Oct 9, 2024

View reviewed changes

Comment thread

src/displayapp/screens/Weather.cpp

		int16_t minTemp = optCurrentForecast->days[i].minTemperature;
		lv_table_set_cell_type(forecast, 2, i, TemperatureStyle(maxTemp));
		lv_table_set_cell_type(forecast, 3, i, TemperatureStyle(minTemp));
		for (int i = 0; i < optCurrentForecast->nbDays; i++) {

FintasticMan Oct 9, 2024

Copy link

Copy Markdown

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

It should be correct to use nbDays rather than MaxNbForecastDays here, but we should test it before merging.

Sorry, something went wrong.

Uh oh!

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

All reactions

JF002 approved these changes

Oct 27, 2024

View reviewed changes

JF002 left a comment

Copy link

Copy Markdown

Collaborator

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.

LGTM!

Sorry, something went wrong.

Uh oh!

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

minacode reacted with hooray emoji

All reactions

🎉 1 reaction

JF002 merged commit f7c87a7 into main

Nov 4, 2024

FintasticMan deleted the temp_to_units branch

November 4, 2024 20:27

Reviewers

minacode

minacode left review comments

JF002

JF002 approved these changes

+2 more reviewers

vkareh

vkareh left review comments

L3br4nd

L3br4nd left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

maintenance

Background work

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

6 participants

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.

Conversation

FintasticMan commented Feb 12, 2024

Uh oh!

github-actions Bot commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

minacode commented Feb 15, 2024

Uh oh!

JF002 commented Feb 15, 2024

Uh oh!

FintasticMan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minacode commented Oct 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JF002 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions Bot commented Feb 12, 2024 •

edited

Loading

Uh oh!

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