Skip to content

Switch SAMD21 ticks to PER event #5100

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 5 commits into from
Aug 12, 2021
Merged

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Aug 5, 2021

The EVSYS is used to generate an interrupt from the event. This
simplifies timing used in pulseio that conflicted with the
auto-reload countdown.

Fixes #3890

@tannewt tannewt added this to the 7.0.0 milestone Aug 5, 2021
@tannewt tannewt requested a review from DavePutz August 5, 2021 22:12
@DavePutz
Copy link
Collaborator

DavePutz commented Aug 5, 2021

I tested this PR using pulsein for a DHT11 on a Metro M0 Express. It consistently failed with too few pulses (between 69 and 73). The protocol is expecting 81 pulses. I will see if I can determine the cause.

@DavePutz
Copy link
Collaborator

DavePutz commented Aug 6, 2021

It looks like the issue is that the line:
RTC->MODE0.COMP[0].reg = target;
in port_interrupt_after_ticks() is taking about 100us to complete. Since the pulses coming in from a DHT11 can be as short as
24us there can be more than one pulse counted as a single pulse. port_interrupt_after_ticks() looks to be getting called about every 525us, so in a ~4ms sequence of incoming pulses there will be several combined values, causing the inaccurate number of pulses. This looks like the same issue mentioned at https://www.avrfreaks.net/comment/2507091#comment-2507091; where modifying the COMP register on the SAMD21 can take up to 180us.

@tannewt
Copy link
Member Author

tannewt commented Aug 6, 2021

Why does the duration of setting interrupt after tick matter? It should be interruptable by the pulsein code.

@DavePutz
Copy link
Collaborator

DavePutz commented Aug 9, 2021

The time it takes to set and synchronize the COMP register is longer than some incoming pulses; and does not seem to be interruptible. This means we cannot afford to do that while an incoming pulse is taking place. As painful as it is, I think we need a way to simply avoid the COMP setting while in a pulse. I did some testing with a boolean flag that returns from port_interrupt_after_ticks() in supervisor/port.c before hitting the COMP statement. It is set in pulsein_interrupt_handler( ) in PulseIn.c when self->first_edge is true (i.e. at the very start of a pulse) and then is reset in common_hal_pulseio_pulsein_popleft() and common_hal_pulseio_pulsein_get_len() when self->len is 0 (i.e. as soon as possible at the the end of a pulse). This seems to work, allowing auto-reloads to work. I realize that there may be a slight delay if the reload gets scheduled while we're in the midst of an incoming pulse; but that window is ~4ms for a DHT and ~68ms for an irremote.

The EVSYS is used to generate an interrupt from the event. This
simplifies timing used in pulseio that conflicted with the
auto-reload countdown.

Fixes micropython#3890
@tannewt
Copy link
Member Author

tannewt commented Aug 10, 2021

Ok, @DavePutz. I added back the COMP set disable. I couldn't find a way to make it not stall the interrupt processing. It doesn't impact ticks anymore though because they use a separate EVSYS interrupt now.

@DavePutz
Copy link
Collaborator

I have found that moving the rtc_start_pulse from common_hal_pulseio_pulsein_construct to the pulsein_interrupt_handler
shortens the pulsein window and lets the auto-reload work more consistently. Patch file I used looks like:


--- a/ports/atmel-samd/common-hal/pulseio/PulseIn.c
+++ b/ports/atmel-samd/common-hal/pulseio/PulseIn.c
@@ -101,6 +101,9 @@ void pulsein_interrupt_handler(uint8_t channel) {
         start_overflow = overflow_count;
     }
     if (self->first_edge) {
+        #ifdef SAMD21
+        rtc_start_pulse();
+        #endif
         self->first_edge = false;
         pulsein_set_config(self, false);
     } else {
@@ -240,9 +243,6 @@ void common_hal_pulseio_pulsein_construct(pulseio_pulsein_obj_t *self,

     // Set config will enable the EIC.
     pulsein_set_config(self, true);
-    #ifdef SAMD21
-    rtc_start_pulse();
-    #endif

 }

Other than that the PR looks good.

Adds CIRCUITPY_BUSIO_UART to disable UART by raising ValueError
that no pins work.
@tannewt
Copy link
Member Author

tannewt commented Aug 11, 2021

@DavePutz I don't want to move it to the interrupt handler because it could be delayed by a sleep. I did add a start/stop in resume/pause so that sleep will be allowed when pulsein is paused.

This leaves much more space on SAMD21 builds that aren't "full builds".
These are new APIs that we don't need to add to old boards.

Also, tweak two Arduino boards to save space on them.
Copy link

@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.

Thank you! No testing performed.

@tannewt tannewt merged commit d294692 into adafruit:main Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

irremote-simpletest.py corrupts auto-reload, and other things on Circuit Playground Express
3 participants