Extend CTS Server

[cybuzuma]

Motivation

I implemented a small TOTP Screen for my watch and thus needed access to UTC. When checking on how to get this information from my phone to the watch, I found and implemented the Local Time Characteristic of the Current Time Service BLE specification. This allows the companion app to transmit information about the current timezone as well as dst in effect.
When checking for related PRs and Issues I found #365 and thought that I'd implement parts of it allowing to show UTC on the watch.

Changes

BLE

• implemented the Local Time Characteristic in CurrentTimeService
• added a check to both Local Time and Current Time to ignore too short data
• corrected the definition of the CtsCurrentTimeData struct in CurrentTimeClient as well as CurrentTimeService to match the specification

DateTimeController

• added fields for timezone and dst offsets to store the values provided by the BLE characteristic
• added getter for UTC offset
• added getter for UTC

Settings

• added UTC option to the clock formats
  

WatchFaces

• changed watch faces to respect UTC setting
• UTC is always shown as 24h format (I do not think someone should display UTC in 12h format, change my mind ;)
• WatchFace Analog: Found no good way to signal that UTC is active, did not want to impact the design
• WatchFace Digital: When displaying UTC the date is shown in ISO format (I guess whoever wants his watch to show UTC is also a big fan of ISO dates :)

• WatchFace Style: Found no good way to signal that UTC is active, did not want to impact the design
• WatchFace Terminal: Shows trailing "Z" on the time if UTC is shown. Also, changed the date to use dashes instead of dots.
  ~~

Tests

I tested my code on the InfiniSim as well as my actual (sealed) watch. I used a patched version of Gadgetbridge to send the added characteristic. So far, I could not observe any problems.

Notes

I added a check for the returncode of the os_mbuf_copydata() function (may for-money job consists a lot of making people not ignore return values, so I am afraid I can not not do it). The question was what to do with invalid data. A) just set the received, potentially invalid, data and let the user see that there is something wrong. B) ignore the data, potentially having the watch not work at all. Seeing that the user can not really do something in any case, I chose B) as it just feels wrong to use invalid data.

I changed the 16bit year field into two 8 bit fields, assembling them manually and making the code more portable. The previous implementation relied on the machine order being little endian. While for now, this code is not very likely to end up on a big endian machine, I really think it is great and could/should end up on a different platform, where this might become an annoying bug.

I kept the separate timezone and dst fields, even if the might not be extremely useful uncombined, but maybe some wants to show if DST is active or something.

For WatchFaceAnalog, I had to pull in <date/date.h> to be able to convert from the chrono information returned by dateTimeController.UTCDateTime(). I could have added UTCHour() and other accessors to DateTimeController, but thought it was cleaner to do the conversion in the WatchFace, seeing the other WatchFaces do as well.

This all depends on a companion app supporting the newly implemented Local Time Characteristic. As I said before, I am using a patched version of Gadgetbridge for myself and I hope I can open a PR there soon (and then look into other companion apps). The question here is, what to do if the companion app does not support it. A) Disable/Hide the UTC option in settings B) leave it, but without effect (all values are initialized to 0, making UTC and local time equivalent). I went with B as I am not a fan of the complexity added by having to check if there was a Local Time update or not, if was valid at some point, and so on. I just think that it is not worth it and a user should be able to tolerate that the setting does not chang anything. Maybe a note can be placed in some documentation (I actually found no in tree documentation on the 24h setting).

[cybuzuma]

Opened corresponding PR at Gadgetbridge

[FintasticMan]

This seems like a useful feature, but I'm not sure if people are going to want to display the time in UTC. I'd happily be proven wrong though!
I can see it useful for, as you mentioned, apps that need access to a fixed time zone, and being able to set a 'home' time zone when travelling.

[cybuzuma]

As I said, my main motivation was to make UTC available to apps.
On the other hand, some persons were supportive of the watch showing UTC in #365.

and being able to set a 'home' time zone when travelling.

Short clarification here: This feature does not change anything about the time displayed (if UTC is not selected). It merely adds the information about the current timezone to the watch's internals. The time displayed will always be the one set by the companion app (which in almost all cases is the local time of the phone/PC). To be able to have the watch display a different timezone than the one of the paired device is a different topic relying more on features of the companion app than the watch itself.

[FintasticMan]

On the other hand, some persons were supportive of the watch showing UTC in #365.

Ah ok, then forget I said anything :)

I do realise that this PR doesn't let you set your own time zone, I was just mentioning that it opens the door to that potential feature in the future.

[Avamander]

I'm ok with having the ability to get UTC or offset, as long as we do not have to deal with timezones. So this PR seems quite fine.

[cybuzuma]

PR against siglo.

[cybuzuma]

PR against itd

[Elara6331]

PR against itd

Please look at my comments on your PR. I am putting this here because my Gitea instance does not send email notifications.

[cybuzuma]

Testing found a missing break.

[Riksu9000]

On the other hand, some persons were supportive of the watch showing UTC in #365.

Maybe a world time clock app would be a better place for this rather than a time format setting. I'm assuming people don't use UTC constantly, so I think people using it would end up going to the settings to change it to UTC, then go to the watchface to look at the time, then go back to the settings to change the format back and finally return to the watchface again. In the end it would probably be easier to just learn the time difference from local time.

[cybuzuma]

Maybe a world time clock app would be a better place for this rather than a time format setting. I'm assuming people don't use UTC constantly, so I think people using it would end up going to the settings to change it to UTC, then go to the watchface to look at the time, then go back to the settings to change the format back and finally return to the watchface again. In the end it would probably be easier to just learn the time difference from local time.

I guess you are right. As I said before, my main point is getting UTC available internally.
It is just when reading #365, I thought I could implement one of the proposals on the way, but I agree that it is not the best UX.

I propose that I remove all changes to the watchfaces and keep this PR simple: Add Local Time characteristic to have UTC available internally.

As a matter of fact, I am already eperimenting with Gadgedbridge and how to get multiple timezones integrated there as well as synced to and displayed in InfiniTime.
What is the best course of action here, should I just implement my vision and create a PR to start a discussion? Or should I present my ideas beforehand for review, and if so, in what form, a WIP PR or an issue or something else?
Thanks for your Feedback!

[FintasticMan]

I propose that I remove all changes to the watchfaces and keep this PR simple: Add Local Time characteristic to have UTC available internally.

I think that sounds good, then a future PR can add something like a time offset from UTC or multiple times from the companion app.

A WIP PR would be a good way to start a discussion around how to handle things.

[cybuzuma]

I updated this PR to the latest development. I did a merge, or do you prefer a rebase?

Also, any update on merging this PR? As said previously, I reverted to only adding the Local Time Characteristic, which as far as I can tell is uncontroversial and a basis for adding other timezones, eg in a world time app.

[FintasticMan]

Usually updating a PR is done with a rebase, but I don't think there is anything wrong with using a merge commit.

[cybuzuma]

as I had to touch it again for clang-format, I squashed and rebased the PR against development.

[cybuzuma]

sry for the failed test, ran clang-format but did not commit the changes -.-

[cybuzuma]

I'm afraid I can not work out why the build failed.
I can build remotes/pull/1161/merge (a4900bf) on my local VM (no docker) with

$ ./arm-none-eabi-gcc --version
arm-none-eabi-gcc (GNU Toolchain for the Arm Architecture 11.2-2022.02 (arm-11.14)) 11.2.1 20220111

and nRF5_SDK_15.3.0_59ac345 without problem.
There is a bug against arm-none-eabi-11.2-2022.02 which hints at a problem with picking up something from the local build machine which might explain why my build succeeds while the docker build does not. Still, as far as I can see, other builds did not fail.

I will rebase my patch onto the current development branch to verify that the problem is not triggered by the implicit merge in the github action...

[FintasticMan]

That bug you mentioned is the problem. Currently ARM recommends just downgrading if it affects you, so I've made a pull request: #1203. At the moment it's just a toss-up whether a build will work or not on GitHub Actions.

[cybuzuma]

Thanks for the info and sorry for not checking beforehand for the PR. Shall I rebase anyway?

[FintasticMan]

If you rebase, it'll allow the build to run again, and it might work that time. But seeing as there are no conflicts and the simulator does build I think it's clear that your code does compile, so I think it's up to you.

If you rebase, a member of the InfiniTimeOrg organisation will have to manually trigger the action.

[cybuzuma]

As I was in the process of doing so anyway, I did a rebase. Thanks for your continuing patience :)

[medeyko]

Is there any problem with this pull request?

I suppose it is important to have the time zone information at least internally, in order to be used in a world clock app or/and for a secondary time zone at watchfaces.

[JF002]

Is there any problem with this pull request?

Nope :) We just needed some time to review it :-)

@cybuzuma Great job! Sorry for the time it took to review it, and thank you for your first contribution! And additional thank you for your contributions in Gadgetbridge, Siglo and ITD. I've commented on those PR to notify the maintainers that the feature is finally merged in InfiniTime :-)