search

jasoncoon / esp8266-fastled-webserver

GNU General Public License v3.0
712 stars 360 forks source link

Added UTC offset to firmware & web app. #213

Closed jasoncoon closed 2 years ago

jasoncoon commented 2 years ago

Fixed pattern order. Fixed fibonacci fire & water orientation. Fixed clock hand angles. Fixed spiral clock patterns.

jasoncoon commented 2 years ago

Feedback welcome @henrygab. Am I doing this right? 😆

henrygab commented 2 years ago

I'll take a look asap (hopefully later tonight).

I've just finished moving to the use of arduinoJson, so I'm quite familiar with fields.cpp right now.

jasoncoon commented 2 years ago

No rush, thanks!

henrygab commented 2 years ago

While I start review, check out this video explaining why timezones are evil.

Then, I will ask you to step back, and explain what aspects of the time zone you want to handle (e.g., offset from UTC? daylight saving time for US? for europe? for every country?)

This can be solved. It's just usually more complex than folks realize, and could come back to bite you later, if you're not intentional in the design at the start.

tobi01001 commented 2 years ago

Hi Jason, Henry,

I do not care if timezones are evil or not and why ;-) but I was somehow triggered by the PR itself. ;-)

I just have some comments / proposals on the implementation: Looking at how the timezones are added in this PR, I was wondering why you did not use a "regular" SelectFieldType like for the patterns or palettes. This would work without the need to change the web app and the mapping could be done on the esp rather than on the client.

cheers, Toby

henrygab commented 2 years ago

I just have some comments / proposals on the implementation: Looking at how the timezones are added in this PR, I was wondering why you did not use a "regular" SelectFieldType like for the patterns or palettes. This would work without the need to change the web app and the mapping could be done on the esp rather than on the client.

Good question ... and I asked the same in my just-finished review (our bits passed each other in the ether).

First, it's important to note that this does not add timezone support ... timezones include more baggage, including daylight saving time. That said...

I think one reason is to provide a unique UI for UTC offset selection. (Not sure if I agree with the UI yet.)

I'd also have to see if the SelectFieldType can provide a value distinct from the string (I don't believe it does)... if it does not, I think it'd make more sense for the UTC field to be generated entirely in the web page, and to just send the UTC offset (_in positive or negative minutes... javascript's native int32_t integer_) to the ESP.

But, that's just my reverse-engineering Jason's apparent reasoning.

tobi01001 commented 2 years ago

I think one reason is to provide a unique UI for UTC offset selection. (Not sure if I agree with the UI yet.)

I'd also have to see if the SelectFieldType can provide a value distinct from the string (I don't believe it does)... if it does not, I think it'd make more sense for the UTC field to be generated entirely in the web page, and to just send the UTC offset (_in positive or negative minutes... javascript's native int32_t integer_) to the ESP.

But, that's just my reverse-engineering Jason's apparent reasoning.

The SelectFieldType just provides an index when set via web app and one just needs to map that index to the desired value and/or action (i.e. the offset in an array of int8 offsets....). https://github.com/jasoncoon/esp8266-fastled-webserver/blob/b3e7937d7cc1dbec6cfa2093b250317a2ba8e753/esp8266-fastled-webserver/data/js/app.js#L274 https://github.com/jasoncoon/esp8266-fastled-webserver/blob/b3e7937d7cc1dbec6cfa2093b250317a2ba8e753/esp8266-fastled-webserver/esp8266-fastled-webserver.ino#L548

So you do work locally just with index->values (array) and textual representation on the web app. This would btw also eliminate the risk of wrong/non-sense offsets...

I did also think about changing fields to uint32 (to be able to have the same behaviour the colorField (working with numbers instead of Strings)... but this will push all other 8bit and boolean values to 32bit (quite a tradeoff)?

henrygab commented 2 years ago

will push all other 8bit and boolean values to 32bit

Yes and no. Yes, it would increase the size of the field array, listing the min/max values. The actual variables would not need to be changed, only some validation when the values are being set externally (e.g., REST API).

tobi01001 commented 2 years ago

will push all other 8bit and boolean values to 32bit

Yes and no. Yes, it would increase the size of the field array, listing the min/max values. The actual variables would not need to be changed, only some validation when the values are being set externally (e.g., REST API).

You are right, I was mixing that up. So it would increase the fileds size quite a bit but not the parameter values itself. Good point. This being said, I think it is generally a good idea to unify this to 32bits because then the getter and setter functions can also be changed to generally provide 32bit values instead of working with Strings... But that is a deifferent discussion unrelated to the PR...

jasoncoon commented 2 years ago

I used a list of UTC time offsets instead of time zones for the very reason that time zones are complicated. A drawback of this approach is that it doesn't handle daylight savings, and the user would need to adjust for that themselves twice a year.

I used uint8_t because that's currently how settings are saved to "EEPROM". Can't (currently) save negative numbers. Hence the list of indexes. mapping, etc. I do see now that I have a problem with fractional hours. I'll get that fixed.

Eventually I would like to change from reading/writing individual bytes in EEPROM and switch to EEPROM.put which could simplify this greatly.

I put the select options in the web app to try to limit bloat in the firmware fields.