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: Extension for the existing port. #8704

Closed
wants to merge 59 commits into from

Conversation

robert-hh
Copy link
Contributor

These are the first two commits restarting PR #8533. The commits are:

  • Remove partially functioning support for REPL duplicated at the UART port. UART support will be added later again with full API coverage.
  • Replace pins.c and pins.h in the board directories by pins.csv. pins.csv is easier to maintain.

@robert-hh robert-hh force-pushed the samd_extend branch 2 times, most recently from a3f6ff8 to d3d61fd Compare May 29, 2022 10:53
@robert-hh
Copy link
Contributor Author

robert-hh commented May 29, 2022

@dpgeorge I suggest the following sequence for commits, which at the end will have the same state as my actual development branch. Each line would be a separate commit with clear focus.

  1. Remove the preliminary REPL on UART support. (PR done)
  2. Replace pins.c and pins.h by pins.csv and make_pins.py. (PR done)
  3. Full clock init, including µs-counter init.
  4. Update mphalport.c and mphalport.h.
  5. Implement Pin Open Drain.
  6. Add SoftI2c and SoftSPI.
  7. Add machine UART consisting of
    • Add the pin_mux table and methods.
    • Rework the ISR table.
    • Add machine.UART and UART ISR.
    • Add uos.dupterm().
  8. Add ADAFRUIT_ITSYBITSY_M0_EXPRESS and ADAFRUIT_FEATHER_M4_EXPRESS
  9. Extensions of the micropython set-up
    • Allow for a board specific manifest.py
    • Use the common way of executing boot.py and main.py.
    • Enable a few more MicroPython features.
  10. Add the class machine.ADC.
  11. Add the class machine.PWM
  12. Add the machine.Timer class based on softtimer.
  13. Add pin.irq()
  14. Small additions to the class machine
    • Enable machine.freq(value) for SAMD51x.
    • Enable machine.disable_irq(), .enable_irq() and .idle().
  15. Add the machine.WDT class to machine
  16. Add uos.urandom() for SAMD51.
  17. Add the machine.SPI class.
  18. Add a pin_info() method to the module samd.
  19. Shrink/Drop the machine.LED class. (optional, to save flash space)
  20. Add the machine.I2C class.

@robert-hh
Copy link
Contributor Author

@dpgeorge Any comments on that approach or the first two commits. Shall I go on with that series or should I wait at each step for your comments?

@robert-hh
Copy link
Contributor Author

So I started to add a few more commits along the steps sketched above. That exercise of pulling the code apart and sorting it again actually improves it.

@robert-hh
Copy link
Contributor Author

robert-hh commented Jun 6, 2022

@dpgeorge This PR is the resorted version of PR #8533. The sequence of commits is slightly change compared to my comment above. The size for a SAMD21 build is now 152k. At an available flash size of 192k, there is now 40 k space left. I prefer to leave ~20k for frozen bytecode or as headroom for source code level debugging of single files. That means, there is about 20k left for code/data. Some package size estimations:

  • floating point support: 12k
  • Asyncio: 1.5k
  • Unicode: 2k

Classes and function on my list:

  • DAC (implemented): ~300 bytes
  • time_pulses (enabled) ~250 bytes
  • Onewire (~750 bytes for the core, ~3k for the python scripts).

I tried to implement bitstream, but the pulse time accuracy was bad.
And there is still the bug in TinyUSB/USB.

@robert-hh
Copy link
Contributor Author

Just rebased to Version 1.19.xx

@robert-hh
Copy link
Contributor Author

Four commits: Three of them a just making a cleaner file structure for SAMD21 vs SAMD51 boards. The 4th is a bypass for a PCB design issue of Adafruit Feather M0/M4 boards, which prevented using the on-board crystal for the MCU clock.

@robert-hh
Copy link
Contributor Author

  • Small changes to existing commits which popped up while writing the documentation. Mostly missing Pin definitions.
  • Removed unneeded code from samd.Flash.
  • Rebased onto the current master.

Porting PR micropython#8040 of @hoihu for SAMD, following the commit
5873390.

One small addition: Before executing keyboard interrupt, the input
buffer is cleared.
From APB_FREQ to DFLL48M_FREQ
From apb_freq to peripheral_freq
For consistency it should be there.
They used to be in mpconfigmcu.h, but have to be different for
different chip variants, like the SAMD51x20.
Somehow that was forgotten.
Using lfs1 gives a smaller code.
@mattytrentini
Copy link
Sponsor Contributor

Thanks @robert-hh - and @dpgeorge for the review - there's a lot of work in this PR and it will be a very welcome addition.

@@ -1,20 +1,20 @@
/*
* This file is part of the MicroPython project, http://micropython.org/
*

Copy link
Member

Choose a reason for hiding this comment

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

something went wrong here, please don't change this license header

@dpgeorge
Copy link
Member

dpgeorge commented Oct 5, 2022

Can be done. Would you like to see more information than the port number?

Would be good to provide the mode and pull state, eg:

Pin(0, mode=OUT, pull=PULL_UP)

@dpgeorge
Copy link
Member

dpgeorge commented Oct 5, 2022

In case of a) there are two commits in the seconds batch, which should be included, because they fix bugs to machine.uart and machine.pwm.

I'm happy to merge this PR with the changes you've made (plus my two new comments above). Those bug fixes can wait until the second batch.

@robert-hh
Copy link
Contributor Author

@dpgeorge The changes are made as requested by updating the previous commits. But I could not stand it leaving PWM as it was. Known inconsistencies should be fixed. So I added the PWM fix. The UART fix is not criticle, because it does not show up in the actual configuration with transmit buffer.

It now prints lines like:

Pin("D9", mode=IN, pull=PULL_UP, GPIO=PA07)

or

LED("LED")

showing for consistency the names as given in pins.csv. For pins,
the GPIO numer is printed as well as reference.
- Clear the Pin AF mode when calling pwm.deinit().
- Defer setting the Pin AF mode until the freq is set.
- Disable a PWM device only if none of it's outputs are used.
- Store the PWM duty in a separate vector. That way, they
  are available at a frequency change for keeping the duty rates.
- Use the double buffered registers for a glitch free freq change.
- Make freq the second (optional) argument of the constructor, not
  requiring a keyword (keyword can still be used).
@dpgeorge
Copy link
Member

dpgeorge commented Oct 5, 2022

Thanks for updating. I should be able to merge this shortly. I may do some squashing of some commits during merge.

@robert-hh
Copy link
Contributor Author

Thanks. No problem about squashing. There are quite a few pretty small commits.

@robert-hh
Copy link
Contributor Author

The matching update for the documentation is in PR #8798.

@dpgeorge
Copy link
Member

dpgeorge commented Oct 6, 2022

Merged in 85afed5 through 366c801

Thank you very much @robert-hh. After going through the commits in detail, it's obvious that a LOT of work went into all of this. It's a really great addition, it makes the samd port close to full-featured.


I squashed a few of the later fix-up commits into the earlier ones. But I only made a very few minor changes overall. For your interest to see what I did change, this is the diff (the changes I made to this PR before merging):

diff --git a/ports/samd/Makefile b/ports/samd/Makefile
index 7510066cf..5ff00f21f 100644
--- a/ports/samd/Makefile
+++ b/ports/samd/Makefile
@@ -90,7 +90,7 @@ SRC_C += \
        mcu/$(MCU_SERIES_LOWER)/clock_config.c \
        help.c \
        machine_adc.c \
-       machine_bitstream.c \
+       machine_bitstream.c \
        machine_dac.c \
        machine_i2c.c \
        machine_led.c \
diff --git a/ports/samd/boards/ADAFRUIT_ITSYBITSY_M4_EXPRESS/pins.csv b/ports/samd/boards/ADAFRUIT_ITSYBITSY_M4_EXPRESS/pins.csv
index 3ad6c4395..ce760a269 100644
--- a/ports/samd/boards/ADAFRUIT_ITSYBITSY_M4_EXPRESS/pins.csv
+++ b/ports/samd/boards/ADAFRUIT_ITSYBITSY_M4_EXPRESS/pins.csv
@@ -13,7 +13,7 @@ PIN_PA14,D4
 PIN_PA15,D5
 -
 PIN_PA18,D7
-- 
+-
 PIN_PA19,D9
 PIN_PA20,D10
 PIN_PA21,D11
diff --git a/ports/samd/boards/SAMD21_XPLAINED_PRO/pins.csv b/ports/samd/boards/SAMD21_XPLAINED_PRO/pins.csv
index 85f76381c..438119d90 100644
--- a/ports/samd/boards/SAMD21_XPLAINED_PRO/pins.csv
+++ b/ports/samd/boards/SAMD21_XPLAINED_PRO/pins.csv
@@ -33,7 +33,7 @@ PIN_PB13,EXT2_PIN8
 PIN_PB14,EXT2_PIN9
 PIN_PB15,EXT2_PIN10
 -
-- 
+-
 PIN_PB11,EXT2_PIN13
 PIN_PB10,EXT2_PIN14
 PIN_PA17,EXT2_PIN15
diff --git a/ports/samd/main.c b/ports/samd/main.c
index 382e7702e..0f24e2382 100644
--- a/ports/samd/main.c
+++ b/ports/samd/main.c
@@ -29,10 +29,10 @@
 #include "py/gc.h"
 #include "py/mperrno.h"
 #include "py/stackctrl.h"
+#include "shared/readline/readline.h"
 #include "shared/runtime/gchelper.h"
 #include "shared/runtime/pyexec.h"
 #include "shared/runtime/softtimer.h"
-#include "shared/readline/readline.h"
 
 extern uint8_t _sstack, _estack, _sheap, _eheap;
 extern void adc_deinit_all(void);
@@ -48,6 +48,7 @@ void samd_main(void) {
     for (;;) {
         gc_init(&_sheap, &_eheap);
         mp_init();
+
         // Initialise sub-systems.
         readline_init0();
 
diff --git a/ports/samd/mcu/samd21/mpconfigmcu.mk b/ports/samd/mcu/samd21/mpconfigmcu.mk
index 1d3b568c6..cc435da8c 100644
--- a/ports/samd/mcu/samd21/mpconfigmcu.mk
+++ b/ports/samd/mcu/samd21/mpconfigmcu.mk
@@ -1,2 +1 @@
-
 SRC_S += shared/runtime/gchelper_m0.s
diff --git a/ports/samd/mcu/samd51/mpconfigmcu.mk b/ports/samd/mcu/samd51/mpconfigmcu.mk
index cc5cb1f31..e83e8911d 100644
--- a/ports/samd/mcu/samd51/mpconfigmcu.mk
+++ b/ports/samd/mcu/samd51/mpconfigmcu.mk
@@ -1,4 +1,3 @@
-
 MICROPY_VFS_LFS2 ?= 1
 
 SRC_S += shared/runtime/gchelper_m3.s
diff --git a/ports/samd/modmachine.c b/ports/samd/modmachine.c
index b10771d47..f6cf7f815 100644
--- a/ports/samd/modmachine.c
+++ b/ports/samd/modmachine.c
@@ -168,10 +168,7 @@ STATIC const mp_rom_map_elem_t machine_module_globals_table[] = {
     { MP_ROM_QSTR(MP_QSTR_disable_irq),         MP_ROM_PTR(&machine_disable_irq_obj) },
     { MP_ROM_QSTR(MP_QSTR_enable_irq),          MP_ROM_PTR(&machine_enable_irq_obj) },
     { MP_ROM_QSTR(MP_QSTR_time_pulse_us),       MP_ROM_PTR(&machine_time_pulse_us_obj) },
-
-    // #if defined (MCU_SAMD51)
     { MP_ROM_QSTR(MP_QSTR_bitstream),           MP_ROM_PTR(&machine_bitstream_obj) },
-    // #endif
 };
 STATIC MP_DEFINE_CONST_DICT(machine_module_globals, machine_module_globals_table);
 
diff --git a/ports/samd/moduos.c b/ports/samd/moduos.c
index 0cddbf748..e0237fd88 100644
--- a/ports/samd/moduos.c
+++ b/ports/samd/moduos.c
@@ -38,7 +38,6 @@
 static bool initialized = false;
 
 STATIC void trng_start(void) {
-
     if (!initialized) {
         MCLK->APBCMASK.bit.TRNG_ = 1;
         REG_TRNG_CTRLA = TRNG_CTRLA_ENABLE;
@@ -47,7 +46,6 @@ STATIC void trng_start(void) {
 }
 
 uint32_t trng_random_u32(void) {
-
     trng_start();
     while ((REG_TRNG_INTFLAG & TRNG_INTFLAG_DATARDY) == 0) {
     }
@@ -73,8 +71,8 @@ STATIC mp_obj_t mp_uos_urandom(mp_obj_t num) {
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(mp_uos_urandom_obj, mp_uos_urandom);
 
-#endif  // MICROPY_PY_UOS_URANDOM
-#endif  // defined(MCU_SAMD51)
+#endif // MICROPY_PY_UOS_URANDOM
+#endif // defined(MCU_SAMD51)
 
 #if MICROPY_PY_UOS_DUPTERM_BUILTIN_STREAM
 bool mp_uos_dupterm_is_builtin_stream(mp_const_obj_t stream) {
diff --git a/ports/samd/samd_soc.c b/ports/samd/samd_soc.c
index 729bd356e..f4a27f3df 100644
--- a/ports/samd/samd_soc.c
+++ b/ports/samd/samd_soc.c
@@ -64,7 +64,7 @@ static void usb_init(void) {
     tusb_init();
 }
 
-// Initialize the µs counter on TC 0/1
+// Initialize the microsecond counter on TC 0/1
 void init_us_counter(void) {
     #if defined(MCU_SAMD21)

@dpgeorge dpgeorge closed this Oct 6, 2022
@robert-hh
Copy link
Contributor Author

Thank you very much. The changes look fine. Now I can proceed with the second set, which is smaller and more maintenance that feature expansion.

@robert-hh
Copy link
Contributor Author

robert-hh commented Oct 6, 2022

I just see that you did some rewriting of the commit messages, which improves them. But you skipped the last commit with the code fixes for PWM. That's a pity. So it will be in the second batch.

I just see that you merged that PWM fix with the initial PWM commit. Thanks.

@dpgeorge
Copy link
Member

dpgeorge commented Oct 6, 2022

I just see that you merged that PWM fix with the initial PWM commit

Yes I squashed it down. I'm quite careful with how I do that, I keep the original branch and constantly do a diff with that and the squashed version to make sure nothing gets lost (see above diff).

@robert-hh
Copy link
Contributor Author

Yes, I noticed on a second glance that you went though the commits piece by piece, and improved the language. It looks much better now.

@robert-hh robert-hh deleted the samd_extend branch October 11, 2022 18:15
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

5 participants