Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Teensy RTC clock class. Make NtpClock compatible with Teensy. #25

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

me21
Copy link

@me21 me21 commented Apr 30, 2024

Also add sync source to SystemClockTemplate to know what was the source the system clock was synced from (Reference, Backup, UserCode or Unknown (not synced)).

Alexander Zarubkin and others added 2 commits April 30, 2024 15:16
… sync source to SystemClockTemplate to know what was the source the system clock was synced from (Reference, Backup, UserCode or Unknown (not synced)).

Signed-off-by: Alexander Zarubkin <alexander.zarubkin@toptal.com>
Rewrite comment.
@bxparks
Copy link
Owner

bxparks commented May 7, 2024

Just a quick note to say that I have not forgotten about this. I'm trying to finish off a feature currently, and I don't want to do a context swap into this code and lose my momentum on my current work.

I took a quick scan through your PR. It's probably been over a year since I looked at this code, so my active knowledge of how my code works is probably pretty rusty, but my high level impression is that you are doing 3 things with this PR:

  1. Add a TeensyRtcClock subclass to support an RTC for a subset of Teensy boards which are not specified (I dont' think specifyingTEENSYDUINO is sufficient since that is defined for the entire spectrum of Teensy boards, from AVR Teensy 2.0 to their most powerful ARM-based Teensy 4.1).
  2. Update NtpClock to support a network library for some subset of Teensy boards, which are not specified.
  3. Add a SyncSource enum to SystemClock whose purpose and enum values are undocumented.

Ideally I would have preferred to have those things done in 3 separate PRs, unless they are coupled in some way that isn't obvious.

My initial request for you, before I dig into this code, is to address the following high level things:

  1. Be more explicit about which Teensy boards are supported for various features. Add links and references to those boards, since I do not have extensive experience with various Teensy boards. I am familiarly basically just the Teensy 3.2, but not the 2.0 or the 4.x boards.
  2. Document the purpose and enum values of the SyncSource enum. In particular, I'm not sure I understand what kSyncSourceUserCode means. The getLastSyncSource() is not used anywhere in this PR, and its documentation does not explain its purpose.
  3. I usually don't include code in my library that I don't explicitly test myself. So if I were to accept this PR, I would like to have more details on which boards were used to test this.

Another relevant background is that I've actually been thinking about dropping support for Teensy boards for most of my libraries going into the future. Mostly because I don't use them, I don't test my code against them, and the Teensyduino development environment is different enough from other 3rd party Arduino platforms that it is laborious and time-consuming to test my code on Teensy boards.

What this means for this PR is that I am looking for a slightly higher level of documentation for Teensy related code, so that I don't have excessive burden of maintaining code that I don't use or test regularly.

Hope this makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants