nrf: Split mpconfigport.h into multiple files. #6246

glennrub · 2020-07-15T20:53:16Z

Splitting mpconfigport.h into multiple device specific
files in order to facilitate variations between devices.

Due to the fact that the devices might have variations in
features and also variations in flash size it makes sense
that some devices offers more functionality than others
without being limited by restricted devices.

For example more micropython features can be activated for
nrf52840 with 1MB flash, compared to nrf51 with 256KB.

glennrub · 2020-07-15T21:06:55Z

One of the main driver for this change is #5472 where i would like to set MICROPY_VFS and a few other configs to be enabled by default on all targets except nrf51 targets. Instead of continue doing ifdefs in mpconfigport.h this is an attempt to clean it a bit up to distinguish feature sets on device level.

dpgeorge · 2020-07-16T00:28:03Z

For stm32 I created a mpconfigboard_common.h to provide default configuration at the board level. Separately, there's alsompconfigport_nanbox.h which is used to configure nan-box builds. Not sure if this helps but might be worth looking at that. Would be good to keep ports consistent in this regard.

glennrub · 2020-07-17T11:47:35Z

For stm32 I created a mpconfigboard_common.h to provide default configuration at the board level.

After looking it a second time, i see that it is quite similar to what i would like to achieve. I believe i'm calling it device instead of board_common . I did not #ifdef the #defines in this PR, as there are no boards that have any variation yet. But as soon as one board deviates an #ifdef-define must be created for that setting. the hiearchy in my mind is (for example for nrf51) mpconfigboard.h -> mpconfigdevice_nrf51.h -> mpconfigport.h. The setting should be possible to override all the way from board.

I could do it a bit like mpconfigboard_common.h in the stm32 port. Keep all settings that can vary between boards in one common file, and let the mpconfigport.h be the more the static one. That would at least clean things up a bit. But one of the goals for me would also be to get it more clear what features are enabled on which board in the end. Specially if nrf52 and nrf91 starts to get a bit more feature rich than nrf51. It could also be solved by structuring the mpconfigboard_common.h :)

I'm believe i cannot make up my mind on my own here. However, I also believe similarities between ports should come first. Let me shuffle it a bit around.

glennrub · 2020-07-18T12:02:00Z

@dpgeorge , It looks to me that stm32 mpconfigboard_common.h is mostly setting variations of MICROPY_HW configuration. While i would more or less want to set MICROPY_PY feature sets. I pushed a commit on top to show a use case on enabling 3 such configurations. Also, I kept the original proposal in the first commit. I realized that my intention of the split might not have been evident, as i only moved a few settings. But, in the new proposal, I have moved a few existing settings, and moved 3 more that I would like to keep different between device families. This time i dropped to move floating point setting and pin count, as it is static to the port and can probably stay in mpconfigport.h.

dpgeorge · 2020-07-18T13:23:54Z

I like your first, original commit. It's pretty clean and clear what it's doing. So I'd suggest go with just that, except maybe remove mpconfigdevice.h and just put what's in there (selecting which device config to include) directly in mpconfigport.h.

glennrub · 2020-07-19T18:33:47Z

Updated the PR with two new commits for a fair comparison. I updated original proposal and also enabled the 3 feature configs to show how it might look. It could probably be optimized to elide the ifndef->define in case a build config/feature is disabled, and default back to py/mpconfig.h. Personally, i believe these duplicates makes it more explicit what features are in or out for each device family. I'm also seeing that in the end most build configs/features are all in these device files (as one or more device would like to enable it), and mpconfigport.h only keeps ifndef->define for config defaults introduced by the port itself. The other content in mpconfigport.h like root table, builtins etc. will stay in mpconfigport.h.

dpgeorge · 2020-07-20T03:10:50Z

Ok, this latest version looks pretty good to me.

dpgeorge · 2020-07-20T03:13:07Z

ports/nrf/mpconfigdevice_nrf51822.h

+// Board overridable feature configuration.
+
+#ifndef MICROPY_PY_ARRAY_SLICE_ASSIGN
+#define MICROPY_PY_ARRAY_SLICE_ASSIGN      (!defined(BLUETOOTH_SD))


it's not allowed to use defined within a #define; instead do:

#define MICROPY_PY_ARRAY_SLICE_ASSIGN (!BLUETOOTH_SD)

or, if the symbolBLUETOOTH_SD is not guaranteed to be defined, probably it should be:

#if defined(BLUETOOTH_SD) #define MICROPY_PY_ARRAY_SLICE_ASSIGN (0) #else #define MICROPY_PY_ARRAY_SLICE_ASSIGN (1) #endif

it's not allowed to use defined within a #define

I did some tests here, and apparently it looks like it does work. Could it be because BLUETOOTH_SD is passed through the compiler as a CFLAG?

However, i get a few 12 extra bytes in the compilation when using (!defined(BLUETOOTH_SD)) compared to just set 0 or 1. I cross checked also thatLTO and DEBUG has no impact on this. It would have looked cleaner doing a single line. I guess, until i can explain those 12 extra bytes, it is more safe to do what you suggest here, doing the second variant as BLUETOOTH_SD might not be present.

I'll post the results here in case it might be worth something (I also cross checked with pca10056, and it is similar compilations).

make BOARD=microbit

mpconfigdevice_nrf51822.h: #define MICROPY_PY_UBINASCII (!defined(BLUETOOTH_SD)) LINK build-microbit/firmware.elf text data bss dec hex filename 142220 100 1156 143476 23074 build-microbit/firmware.elf firmware.map: 0x000000000001d67c mp_module_ubinascii

mpconfigdevice_nrf51822.h: #define MICROPY_PY_UBINASCII (1) LINK build-microbit/firmware.elf text data bss dec hex filename 142232 100 1156 143488 23080 build-microbit/firmware.elf firmware.map: 0x000000000001d67c mp_module_ubinascii

mpconfigdevice_nrf51822.h: #define MICROPY_PY_UBINASCII (0) LINK build-microbit/firmware.elf text data bss dec hex filename 141264 100 1156 142520 22cb8 build-microbit/firmware.elf firmware.map: Symbol mp_module_ubinascii not found

make BOARD=microbit SD=s110

mpconfigdevice_nrf51822.h: #define MICROPY_PY_UBINASCII (!defined(BLUETOOTH_SD)) LINK build-microbit-s110/firmware.elf text data bss dec hex filename 149088 100 1300 150488 24bd8 build-microbit-s110/firmware.elf firmware.map: Symbol mp_module_ubinascii not found

#define MICROPY_PY_UBINASCII (1) LINK build-microbit-s110/firmware.elf text data bss dec hex filename 150048 100 1300 151448 24f98 build-microbit-s110/firmware.elf firmware.map: 0x0000000000036a00 mp_module_ubinascii

mpconfigdevice_nrf51822.h: #define MICROPY_PY_UBINASCII (0) LINK build-microbit-s110/firmware.elf text data bss dec hex filename 149100 100 1300 150500 24be4 build-microbit-s110/firmware.elf firmware.map: Symbol mp_module_ubinascii not found

I did some tests here, and apparently it looks like it does work

It could just be gcc that allows it. I'm pretty sure it's invalid code by the C standard and clang or other compilers will reject it. Might not be an issue for the nrf port with arm-none-eabi-gcc but IMO better to avoid this pattern.

However, i get a few 12 extra bytes in the compilation when using (!defined(BLUETOOTH_SD)) compared to just set 0 or 1

From the output above it looks like the 12 extra bytes are there when using 0/1 (ie the opposite of your statement).... and that could be just because the git repo is dirty and it adds the -dirty string to the REPL banner, which increases code size.

From the output above it looks like the 12 extra bytes are there when using 0/1 (ie the opposite of your statement).... and that could be just because the git repo is dirty and it adds the -dirty string to the REPL banner, which increases code size.

Yeah, that makes sense! Thanks! I had forgot about the appended string. The count also indicates this.

Splitting mpconfigport.h into multiple device specific files in order to facilitate variations between devices. Due to the fact that the devices might have variations in features and also variations in flash size it makes sense that some devices offers more functionality than others without being limited by restricted devices. For example more micropython features can be activated for nrf52840 with 1MB flash, compared to nrf51 with 256KB.

Enabling the following features for all targets, except for nrf51 targets compiled to be used with SoftDevice: - MICROPY_PY_ARRAY_SLICE_ASSIGN - MICROPY_PY_SYS_STDFILES - MICROPY_PY_UBINASCII

glennrub · 2020-07-21T10:14:11Z

Removed development commits. I guess the PR is now what i would like to get in.
Does it look OK @dpgeorge ?

Edit: removed "more or less" :)

dpgeorge · 2020-07-21T11:39:26Z

Looks good!

glennrub · 2020-07-22T11:01:32Z

Merged in 0a79e18 and caaaa2b.

Don't reset GPIO4 on the MagTag (used for voltage monitoring)

dpgeorge added the port-nrf label Jul 16, 2020

glennrub force-pushed the split_mpconfigport branch 2 times, most recently from f5a38d1 to 3636eeb Compare July 18, 2020 11:46

dpgeorge reviewed Jul 20, 2020

View reviewed changes

glennrub added 2 commits July 20, 2020 12:52

nrf: Enable more features for all targets.

33add63

Enabling the following features for all targets, except for nrf51 targets compiled to be used with SoftDevice: - MICROPY_PY_ARRAY_SLICE_ASSIGN - MICROPY_PY_SYS_STDFILES - MICROPY_PY_UBINASCII

glennrub force-pushed the split_mpconfigport branch from c555c77 to 33add63 Compare July 21, 2020 10:07

glennrub closed this Jul 22, 2020

glennrub deleted the split_mpconfigport branch July 22, 2020 11:04

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request May 12, 2022

Merge pull request micropython#6246 from DavePutz/issue_6148

dbb6f5f

Don't reset GPIO4 on the MagTag (used for voltage monitoring)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

nrf: Split mpconfigport.h into multiple files. #6246

nrf: Split mpconfigport.h into multiple files. #6246

Uh oh!

glennrub commented Jul 15, 2020

Uh oh!

glennrub commented Jul 15, 2020

Uh oh!

dpgeorge commented Jul 16, 2020

Uh oh!

glennrub commented Jul 17, 2020

Uh oh!

glennrub commented Jul 18, 2020

Uh oh!

dpgeorge commented Jul 18, 2020

Uh oh!

glennrub commented Jul 19, 2020

Uh oh!

dpgeorge commented Jul 20, 2020

Uh oh!

dpgeorge Jul 20, 2020

Uh oh!

glennrub Jul 20, 2020

Uh oh!

dpgeorge Jul 20, 2020

Uh oh!

glennrub Jul 21, 2020

Uh oh!

glennrub commented Jul 21, 2020 •

edited

Loading

Uh oh!

dpgeorge commented Jul 21, 2020

Uh oh!

glennrub commented Jul 22, 2020

Uh oh!

Uh oh!

Uh oh!

nrf: Split mpconfigport.h into multiple files. #6246

nrf: Split mpconfigport.h into multiple files. #6246

Uh oh!

Conversation

glennrub commented Jul 15, 2020

Uh oh!

glennrub commented Jul 15, 2020

Uh oh!

dpgeorge commented Jul 16, 2020

Uh oh!

glennrub commented Jul 17, 2020

Uh oh!

glennrub commented Jul 18, 2020

Uh oh!

dpgeorge commented Jul 18, 2020

Uh oh!

glennrub commented Jul 19, 2020

Uh oh!

dpgeorge commented Jul 20, 2020

Uh oh!

dpgeorge Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

glennrub Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

dpgeorge Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

glennrub Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

glennrub commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpgeorge commented Jul 21, 2020

Uh oh!

glennrub commented Jul 22, 2020

Uh oh!

Uh oh!

glennrub commented Jul 21, 2020 •

edited

Loading