Added UTC offset to firmware & web app.

[jasoncoon]

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

[jasoncoon]

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

[henrygab]

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]

No rush, thanks!

[henrygab]

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]

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]

I don't think this code does what you'd expect. Test index 1, 2, and 3, for example, and see the offsets that get applied.

Even if they did what was intended, I'm still recommending changing from utcOffsetIndex to passing and saving utcOffsetMinutes directly. It makes the source code applying the value much easier to understand. Javascript's natively going to send int32_t of data, so it can easily handle minute-based offsets. The field structure's min and max would need to be updated from uint8_t to int32_t, which can be backwards-compatible.

[henrygab]

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]

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

esp8266-fastled-webserver/esp8266-fastled-webserver/data/js/app.js

Line 274 in b3e7937

postValue(field.name, value);

esp8266-fastled-webserver/esp8266-fastled-webserver/esp8266-fastled-webserver.ino

Line 548 in b3e7937

sendInt(currentPatternIndex);

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]

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]

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]

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.