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

samd: Some improvements and bug fixes. #10021

Merged
merged 9 commits into from
Dec 14, 2022
Merged

Conversation

robert-hh
Copy link
Contributor

@robert-hh robert-hh commented Nov 19, 2022

  • Support MICROPY_HW_SOFTSPI_MIN_DELAY, increasing the SoftSPI baudrate to about 500kHz.
  • Use __WFE() in the MICROPY_EVENT_POLL_HOOK, which reduces the wait time at the end of SPI read from up to 1 ms to a few µs.
  • Avoid under-/overflow in the baudrate calculation. The underflow caused a baud rate setting of 48MHz resulting in a baud rate of ~100kHz.
  • Fix UART IRQ flag setting and clearing. Without that fix, writing UART would disable the UART receive interrupt.
  • Simplify machine_uart.c, by removing the call to uart_drain_rx_fifo() from machine_uart_any() and machine_uart_read(). It is not required, and may cause a race condition.
    method. Simplifies _boot.py as well
  • Check the UART tx Pin assignment.
  • Fix uart.deinit() and do not use the finaliser with the machine_uart object.
  • Add a vref=n option to machine.ADC and machine.DAC. Some boards do not have the AREF input wired, which was used as default input. With the vref option, the reference source can be selected, e.g. to Vcc or to the internal reference.
  • Support touch1200bps to start bootloader mode.

The first three changes are more inconveniences, which could be avoided in the Python scripts or "just" affect the timing. Nr. 4, 5, and 7 fix bugs, Nr. 6 is an additional test, Nr. 8 is for some boards a bug fix and in general an extension.

@@ -123,7 +125,7 @@ __attribute__((always_inline)) static inline uint32_t disable_irq(void) {
do { \
extern void mp_handle_pending(bool); \
mp_handle_pending(true); \
__WFI(); \
__WFE(); \
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this works, because there are no SEV calls anywhere in this port, so WFE can never be woken.

I would prefer to use WFE rather than WFI because you have more control over when to wake the CPU, and it's easier to avoid race conditions. Using WFI you usually need to do this:

void wait(void) {
    for(;;) {
        disable_irqs;
        if (event/interrupt of interest occurred) {
            enable_irqs;
            return;
        }
        WFI;
        enable_irqs;
    }
}

WFE on its own can effectively replace the above function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I understood it, WFE also responds to a hardware interrupt, and maybe other hardware events, like from the . The difference here is, that with WFI the SPI unit wait for the next Systick interrupt after the last byte has been transferred, but using WFE it finishes immediately. That's why I replaced WFI by WFE.
Both instructions result in the same power consumption in REPL.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I guess the SAMD architecture has its own way of handling events/interrupts (I think the MCU can decide how to handle them, it's not part of the ARM Cortex spec, although I could be wrong there).

@robert-hh robert-hh force-pushed the samd_sp2 branch 2 times, most recently from d63a0a4 to 89a0c5c Compare November 23, 2022 18:02
@robert-hh robert-hh marked this pull request as draft November 25, 2022 11:19
@robert-hh robert-hh force-pushed the samd_sp2 branch 2 times, most recently from 0aa9e3b to 7b2c160 Compare November 25, 2022 15:12
@robert-hh robert-hh changed the title samd: Some improvements and a bug fix. samd: Some improvements and bug fixes. Nov 25, 2022
@robert-hh robert-hh marked this pull request as ready for review November 25, 2022 15:31
@robert-hh robert-hh force-pushed the samd_sp2 branch 2 times, most recently from df52f78 to 9f26fd5 Compare December 1, 2022 20:04
@robert-hh robert-hh force-pushed the samd_sp2 branch 3 times, most recently from ed062c4 to 11bea2c Compare December 9, 2022 20:19
Bringing the SoftSPI baudrate up to about 500 kHz.
Like WFI, WFE also responds to a hardware interrupt, and using WFE speeds
up at least spi.read().  Power consumption at an idle REPL is unchanged.
Applies to both SPI and I2C.  The underflow caused high baudrate settings
resulting in the lowest possible baudrate.  The overflow resulted in
erratic baudrates, not just the lowest possible.
Clearing the DRE flag for the transmit interrupt at the end of a
uart.write() also cleared the RXC flag disabling the receive interrupt.

This commit also changes the flag set/clear mechanism in the driver for SPI
as well, even if it did not cause a problem there.  But at least it saves a
few bytes of code.
Remove the call to uart_drain_rx_fifo().  It is not required, and may cause
a race condition.
Check, if TX is at Pad 0 (SAMD51), or Pad 0 or 2 (SAMD21).
Changes in this commit:
- Do not deinit IRQ when uart.deinit() is called with an inactive object.
- Remove using it for the finaliser.  There is another machanism for soft
  reset, and it is not needed otherwise.
- Do not tag the UART buffers with MP_STATE_PORT, it is not required.
ADC: The argument of vref=num is an integer. Values for num are:

    SAMD21:
    0  INT1V   1.0V voltage reference
    1  INTVCC0 1/1.48 Analog voltage supply
    2  INTVCC1 1/2 Analog voltage supply (only for VDDANA > 2.0V)
    3  VREFA   External reference
    4  VREFB   External reference

    SAMD51:
    0  INTREF  internal bandgap reference
    1  INTVCC1 Analog voltage supply
    2  INTVCC0 1/2 Analog voltage supply (only for VDDANA > 2.0v)
    3  AREFA   External reference A
    4  AREFB   External reference B
    5  AREFC   External reference C (ADC1 only)

DAC: The argument of vref=num is an integer. Suitable values:

    SAMD21:
    0  INT1V   Internal voltage reference
    1  VDDANA  Analog voltage supply
    2  VREFA   External reference

    SAMD51:
    0  INTREF Internal bandgap reference
    1  VDDANA Analog voltage supply
    2  VREFAU Unbuffered external voltage reference (not buffered in DAC)
    4  VREFAB Buffered external voltage reference (buffered in DAC).
@dpgeorge dpgeorge merged commit 17ab2f6 into micropython:master Dec 14, 2022
@dpgeorge
Copy link
Member

Thank you! And thanks for splitting changes into neat commits.

Rebased and merged with very minor changes to formatting.

@robert-hh
Copy link
Contributor Author

Thank you for merging.

@robert-hh robert-hh deleted the samd_sp2 branch December 15, 2022 07:40
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.

None yet

2 participants