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.

Sign up for GitHub

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

Jump to bottom

use supervisor.ticks_ms when available instead of time.monotonic() #68

Merged
merged 2 commits into from
Dec 3, 2021
Merged

use supervisor.ticks_ms when available instead of time.monotonic() #68

merged 2 commits into from
Dec 3, 2021

Conversation

jerryneedell
Copy link
Contributor

@jerryneedell jerryneedell commented Dec 1, 2021 •
edited by jepler
Loading

Thank you to https://github.com/mrdalgaard for suggesting this.
Closes: #67
This change will use supervisor.ticks_ms() instead of time.monotonic() for the timeout checks.
This will avoid potential issues with time.monotonic() losing accuracy with time.

For the Raspberry Pi implementation via Blinka, the supervisor module is not preset so time.monotonic() is still used but the Pi does not suffer for the same degradation of the accuracy of time.monotonic() so it should not be an issue there.

I have tested this on a feather_rp2040 with rfm9x featherwing and on an unexpected maker feathers2 with rfm9x reatherwing as well as on a Raspberry P4 with an rfm9x bonnet.

If everyone is happy with this change, it should be tested there prior to merge.

@jerryneedell jerryneedell requested review from brentru and a team December 1, 2021 16:40
@jerryneedell jerryneedell mentioned this pull request Dec 1, 2021
Closed
@dhalbert
Copy link
Contributor

dhalbert commented Dec 1, 2021

supervisor is not currently in Blinka, but might we add a subset implementation that includes tick_ms()?

@mrdalgaard
Copy link

Tested on nRF with high time.monotonic() value.
send_with_ack now responds correctly to subsecond ack_wait values, which it did not before.

@jepler
Copy link
Member

jepler commented Dec 1, 2021

I changed the initial comment to say "Closes: #67" so that the issue will be automatically closed if this is merged. https://docs.github.com/en/github/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue for more information

@jepler
Copy link
Member

jepler commented Dec 1, 2021

https://github.com/adafruit/Adafruit_CircuitPython_Ticks/blob/main/adafruit_ticks.py provides an implemenation of ticks_ms that is intended to work on standard python / blinka. I am guessing this was not used to avoid growing the dependencies of this lib...?

@mrdalgaard
Copy link

https://github.com/adafruit/Adafruit_CircuitPython_Ticks/blob/main/adafruit_ticks.py provides an implemenation of ticks_ms that is intended to work on standard python / blinka. I am guessing this was not used to avoid growing the dependencies of this lib...?

Good point. PR will also break on < CP 7.0 versions, as supervisor is available, but supervisor.ticks_ms() is not.

@jerryneedell
Copy link
Contributor Author

https://github.com/adafruit/Adafruit_CircuitPython_Ticks/blob/main/adafruit_ticks.py provides an implemenation of ticks_ms that is intended to work on standard python / blinka. I am guessing this was not used to avoid growing the dependencies of this lib...?

It was not used because I was not aware of it ... I'll rework it to use the library. I am a bit concerned about the impact on the feather_m0_rfm9x build, but it's worth a try.

@jerryneedell
Copy link
Contributor Author

Would using adafruit_ticks be considered a "breaking change" ?

@jerryneedell
Copy link
Contributor Author

https://github.com/adafruit/Adafruit_CircuitPython_Ticks/blob/main/adafruit_ticks.py provides an implemenation of ticks_ms that is intended to work on standard python / blinka. I am guessing this was not used to avoid growing the dependencies of this lib...?

Good point. PR will also break on < CP 7.0 versions, as supervisor is available, but supervisor.ticks_ms() is not.

ugh -- good catch

@jerryneedell
Copy link
Contributor Author

Would it be OK to only import adafruit_ticks if supervisor.ticks_ms() is not available? That might help with both problems and reduce the code size for the smaller build.

@jerryneedell
Copy link
Contributor Author

On second thought, I really don't see the need for adafruit_ticks on the Raspberry Pi.
We should check if supervisor.ticks_ms() exists -- and if not use time.monotonic on the MCUs.
Does that make sense?

@jerryneedell
Copy link
Contributor Author

Does this look better? -- it now only uses tick_ms() if it exists in supervisor - else it uses time.monotonic()

@jerryneedell
Copy link
Contributor Author

I verified that the feather_m0_rfm9x build still fits and ran one of the test examples on a feather_m0_rfm9x

jepler
jepler approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Looks fine to me, given the constraints you're working in. Thank you.

@jerryneedell jerryneedell merged commit 0df4521 into adafruit:main Dec 3, 2021
@jerryneedell jerryneedell deleted the jerryn_ticks branch December 3, 2021 16:49
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Dec 9, 2021
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@628f852)
572bac4 
Updating https://github.com/adafruit/Adafruit_CircuitPython_BME680 to 3.4.0 from 3.3.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_BME680#43 from caternuson/bme688_update
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_CharLCD to 3.3.12 from 3.3.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_CharLCD#70 from dhalbert/make-package
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_DS3231 to 2.4.10 from 2.4.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_DS3231#39 from tekktrik/docfix/correct-example-wday-num
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP_ATcontrol to 0.6.1 from 0.6.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_ESP_ATcontrol#51 from dhalbert/make-package
  > Merge pull request adafruit/Adafruit_CircuitPython_ESP_ATcontrol#50 from mperino/main
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_FRAM to 1.3.10 from 1.3.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_FRAM#28 from tekktrik/fix/slice-notation
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_OV5640 to 1.0.3 from 1.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_OV5640#10 from adafruit/stop-motion-example
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM69 to 2.1.6 from 2.1.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_RFM69#41 from jerryneedell/jerryn_ticks
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM9x to 2.2.2 from 2.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_RFM9x#68 from jerryneedell/jerryn_ticks
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_AdafruitIO to 5.6.0 from 5.5.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#81 from kattni/create-and-get-feed
  > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#77 from aerialist/main
  > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#80 from dhalbert/make-examples-packages
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_asyncio to 0.5.1 from 0.5.0:
  > fix exception printing

Updating https://github.com/adafruit/Adafruit_CircuitPython_binascii to 1.2.7 from 1.2.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_binascii#13 from tekktrik/feature/add-typing
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_Layout to 1.15.0 from 1.14.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_Layout#58 from FoamyGuy/cell_anchor_point

Updating https://github.com/adafruit/Adafruit_CircuitPython_FunHouse to 2.1.8 from 2.1.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_FunHouse#21 from RufusVS/tone_frequency_0_fix
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_Gizmo to 1.3.6 from 1.3.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_Gizmo#17 from caternuson/iss22
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_MagTag to 2.1.3 from 2.1.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MagTag#74 from dhalbert/make-package
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_MatrixPortal to 3.0.3 from 3.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MatrixPortal#81 from dhalbert/make-package
  > update rtd py version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jepler jepler jepler approved these changes

@brentru brentru Awaiting requested review from brentru

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: Switch time.monotonic() to supervisor.ticks_ms()
4 participants
@jerryneedell @dhalbert @mrdalgaard @jepler