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

guard against invalid response #38

Closed
wants to merge 2 commits into from

Conversation

tedder
Copy link
Contributor

@tedder tedder commented Jul 16, 2024

Test to ensure response value is valid. Intermittent seconds=0 value have been seen.

fixes #35

@tedder tedder mentioned this pull request Jul 16, 2024
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you seen a old value that isn't zero? I think it was zero in your example. That might be a better case to detect than this one.

@tedder
Copy link
Contributor Author

tedder commented Jul 17, 2024

I've only seen zero. Could do that if it's what you'd like.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think detecting zero would be clearer. Thanks!

adafruit_ntp.py Outdated Show resolved Hide resolved
@tedder
Copy link
Contributor Author

tedder commented Jul 23, 2024

This may get usurped by #39, which is totally fine.

Test to ensure response value is valid. Intermittent seconds=0 value have been seen.

fixes adafruit#35
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simpler! Thank you!

@tannewt
Copy link
Member

tannewt commented Jul 24, 2024

Do you want to update this since #39 is merged?

@tedder
Copy link
Contributor Author

tedder commented Jul 24, 2024

I don't know that the failure will be the same now that 39 is merged. It's probably these unpackings: https://github.com/adafruit/Adafruit_CircuitPython_NTP/pull/39/files#diff-2476a2f533d178076a598d69ec1dcb7c111cb00afe72382d2d68dcab2d9d0c8cR103

but I'm not confident on what's going to happen. So this can be closed.

@tannewt tannewt closed this Jul 25, 2024
@jepler
Copy link
Member

jepler commented Jul 26, 2024

I know this is closed, but I just wanted to note that the seconds value of 0 is special in NTP:

Timestamps are unsigned values, and operations on them produce a
result in the same or adjacent eras. Era 0 includes dates from the
prime epoch to some time in 2036, when the timestamp field wraps
around and the base date for era 1 is established. In either format,
a value of zero is a special case representing unknown or
unsynchronized time.

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.

OverflowError
3 participants