Skip to content

Switch to time.monotonic_ns() when it's available #10

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

Merged
merged 12 commits into from
Apr 16, 2020

Conversation

tgs
Copy link
Contributor

@tgs tgs commented Aug 13, 2019

Hi again!
Here's the work I've done so far. I wrote tests to make sure I wasn't introducing breaking changes (that I could think of... do you see any?). There doesn't seem to be a CircuitPython standard testing library, so I just wrote some minimal version inside the tests.py file. If you don't want to merge in the tests, that would be OK.

After 3 days of on-time, the current version of the debouncer code fails test_simple on line 25 - the time returned by monotonic() hasn't ticked yet in the .02 seconds of sleep. The new version passes all the tests. I've run it on an ItsyBitsy M0 Express at boot and at 2 and 3 days, and a Trinket M0 at boot (where it doesn't have monotonic_ns).

Let me know what you think!

This should close issue #9.

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.

Looks awesome! Thank you!

@tannewt
Copy link
Member

tannewt commented Aug 16, 2019

@tgs
Copy link
Contributor Author

tgs commented Aug 17, 2019 via email

@tannewt
Copy link
Member

tannewt commented Aug 19, 2019

Great! Thank you in advance! Hope you had a good weekend.

Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! There are two Pylint complaints along with the need to move tests.py into a /tests/ directory. @tgs, if you're still available, please consider making these changes to this PR so we can get it added to the library.

@tgs
Copy link
Contributor Author

tgs commented Jan 10, 2020

Returning to this after a while, I notice that the previous_time attribute's semantics have changed (it's now in nanoseconds when available). I think I will make it private. I could provide a @property version of it if you think that's important?

I'm also changing a bunch of names of internal stuff to try to keep it clear what's measured in "whatever the monotonic clock gives us" (which I'll call ticks), and what's in seconds. I'm sure you'll tell me if that actually makes things more confusing!

@kattni
Copy link
Contributor

kattni commented Jan 29, 2020

@tgs Thank you so much for getting this fixed up! I apologise for the delayed response - I managed to miss your updates. I'm going to have someone test this before merging, but we'll get it merged ASAP!

@tgs
Copy link
Contributor Author

tgs commented Jan 29, 2020

No prob! Sounds good :)

@FoamyGuy
Copy link
Contributor

I've got two ItsyBitsy M0s set aside to run this test for a few days. I should be able to begin within the next day or two, I just need to get PCs set up to watch the log output from them during testing.

@FoamyGuy
Copy link
Contributor

Testing has begun, I'll report back in a few days the results.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Feb 5, 2020

I do have this running side by side on two Itsy Bitsy M0s, but I am having a bit of trouble with them getting reset by static and by the PC they are plugged into. Which I assume is also resetting the "timer" for how long the issue takes to show itself. Since the PC seems to be a bit trigger happy about writing something, or accessing the drive in some way that is making the Circuit Py devices soft reboot as if a file has changed, today I am going to work on trying to set them up have write access from the python code. Then I will try to catch and save the exception message to a file so we can confirm after the fact that it's the same thing noticed originally for this issue. With that in place I will power them from the wall so it won't get interrupted by the PC again.

@tgs
Copy link
Contributor Author

tgs commented Feb 5, 2020

Cool! My experience was that the counter would keep increasing as long as they were powered on - I didn't have to be in the python REPL or anything, and soft reboots didn't reset the counter. But that's my recollection from months ago.

Just to clarify, you have two running so you can run the same test with the patched and unpatched versions of the library code?

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Feb 5, 2020

Ah, thank you. I'll leave them plugged in for now then rather than switch over then. That is correct, one running the patched lib and one running unpatched.

@FoamyGuy
Copy link
Contributor

Another update on this, I have yet to have either the master code, or the updated code actually crash. The only difference that I noticed is anecdotal only (don't have any measuring equipment hooked up or anything) but after several days of running I noticed that the master code device had to have the button pressed down for just a bit longer than the updated code in order for it to register the change from the debounce API.

The PC eventually ended up resetting both devices again, and after the reset there is no more noticeable difference in the time it takes for the button press to register.

I'm going to move the testing devices off the PC to power from the wall. I won't be able to see the stack trace if/when it crashes but it should not get reset nearly as aggressively. But I am curious to see if after a longer time the difference between the two will get more pronounced and if I can manage to see the master code one crash.

@kattni
Copy link
Contributor

kattni commented Mar 30, 2020

@tgs @FoamyGuy Please post a current status here. As well, we have made changes to the library that now require an update to this PR to resolve merge conflicts. Please let us know if you need assistance with this.

@FoamyGuy
Copy link
Contributor

I do still have both Itsy Bitsy M0s running. I did have a few more unrelated resets shortly after the last post and I moved them to a different outlet.

It's run for about 4 weeks solid with no resets or any other issues. I've still never had either device (main branch / edited code) crash.

It does seem like the edited code reacts a bit quicker to the button presses than the main branch code starting after a few days of running. But both have slowed over time as well.

@kattni
Copy link
Contributor

kattni commented Apr 16, 2020

I resolved the merge conflict. However, this PR is now failing on the Black check. Running Black is trivial, but is required for this PR to pass.

@tgs Are you available to update the PR? I'm happy to explain how to run Black.

@tgs
Copy link
Contributor Author

tgs commented Apr 16, 2020

Hi @kattni, I will take a look at the formatting problem. Thanks for fixing the merge conflict!

@kattni
Copy link
Contributor

kattni commented Apr 16, 2020

@tgs Great thanks! I'll get this merged! Thank you for your patience and perseverance!

@tgs
Copy link
Contributor Author

tgs commented Apr 16, 2020

Thanks, that's great! I appreciate your help :)

@kattni kattni merged commit d31993e into adafruit:master Apr 16, 2020
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 21, 2020
@tgs tgs deleted the use-monotonic-ns branch April 24, 2020 15:43
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.

4 participants