nRF52 update with internal file system support#496
Conversation
jerryneedell
left a comment
There was a problem hiding this comment.
Thanks for submitting this - I have been testing it from your repo directly.
I'm curious why you have BLE_MODULE and UBLUEPY_MODULE comment out at line 275 in mpconfigport.h.
I cloned this version from your repo and uncommented them and it built OK and I was able to run some simple BLE tests. Of course I can do that with this version as well, but I was curious why they were omitted.
|
I haven't done any meaningful BLE tests yet simply to keep the list of problems manageable at the start, and avoid the SD for the moment. The next item on the ToDo list is making sure SDv132 2.x and 5.x work fine, and playing with BLE support. Good to hear it works for you, though, and thanks for testing! |
|
I merged this PR in my local clone of the repo. The feather52 build executed fine, but th boot-flash build fails: Any idea what is going wrong? This worked for me in my clone of microbuiulders repo |
|
There seems to be some sort of file property issue with the .zip files. Works fine for me locally: Can you try replacing these two files locally and see if this works for you ( feather52_bootloader_2.0.1_s132_single.zip |
|
Ah - found the problem - the file feather52_bootloader_2.0.1_s132_single.zip in boards/featther52/bootloader appears to be corrupted - I replaced it with the file from microbuilders repo and am able to load the boot-flash and continue - everything is working fine. I recall this happening on an earlier version as well. |
|
Our posts crossed ;-) That was the problem - thanks |
|
I noticed that if I upload a .py file to the file system via ampy and then execute it via the REPL: then then REPL does not respond to a Control -C to interrupt the execution as expected. Note: If I execute the code via ampy run then control-c works as expected. |
|
additional info - even if I cut/paste the code into the REPL (using control-E) I cannot interrupt with control-c 60.98617 cannot interrupt |
|
simpler example from REPL: cannot interrupt |
|
I can reproduce that here, thanks for the heads up. |
tannewt
left a comment
There was a problem hiding this comment.
Very excited to see this! A number of minor comments. Nothing major.
| lib/utils/pyexec.c \ | ||
| lib/libc/string0.c \ | ||
| lib/mp-readline/readline.c \ | ||
| internal_flash.c \ |
There was a problem hiding this comment.
Is this indentation weird because of tabs vs spaces?
| @@ -1,14 +1,18 @@ | |||
| # Setup | |||
There was a problem hiding this comment.
Is this file more general? Could it be merge with ports/nrf/README.md?
There was a problem hiding this comment.
This is mostly specific to the feather52 board (bootloader changes), and doesn't apply to the micro:bit or any other boards we are likely to support such as the PCA10040.
There was a problem hiding this comment.
Are you also planning support for the PCA10056 board?
There was a problem hiding this comment.
It's on our radar due to USB and since this is currently the only board available (widely) with this chipset.
There was a problem hiding this comment.
Ok its up to you. I worry this location may be harder to find than a board specific section in the port README.
|
|
||
| #include "boards/board.h" | ||
|
|
||
| #if 0 |
There was a problem hiding this comment.
Are you expecting to need these later? If you are, generally I comment these out with // when I don't need them because my editor makes it easy to uncomment and comment a number of lines. Otherwise, if you don't expect to need them, I'd recommend removing them.
| NRF_RTC1->TASKS_START = 1; | ||
| NRF_RTC1->EVTENSET = RTC_EVTEN_OVRFLW_Msk; | ||
| NVIC_SetPriority(RTC1_IRQn, 0xf); // lowest priority | ||
| NVIC_EnableIRQ(RTC1_IRQn); |
There was a problem hiding this comment.
In the atmel-samd port we handle this in the port level init and any high level clock config is done through defines. Are the existing boards different enough to warrant this done per board?
Additionally, should we remove the other dev boards we don't plan on supporting?
There was a problem hiding this comment.
I think we should remove other boards that we definitely won't be able to support, but I'd like to define what that list should be first so have kept things as is for the moment, and the files already exist in master so I think we can kick that can down the road to the next PR. They should go if we can't support them, though, especially with the API level changes required for maintenance.
Clock support on the nRF series does actually vary a lot board to board depending on HW design since there are several valid clock sources and configs. Some of our boards have a 32kHz XTAL for low power mode, some don't, etc., and I think clock setup with a low power chip like this generally is very board specific. That can be moved to macros, but I'm not convinced there's any big gain doing that except having to jump between files???
There was a problem hiding this comment.
I guess I'm thinking towards a world where we have multiple board designs of our own with nrf52 just like the M0. Its fine with me to wait until we actually need it refactored.
| /* produce a link error if there is not this amount of RAM for these sections */ | ||
| _minimum_stack_size = 2K; | ||
| _minimum_heap_size = 16K; | ||
| _minimum_heap_size = 0 /*16K Circuit Python use static variable for HEAP */; |
There was a problem hiding this comment.
Yup yup, and in the future I was to play with dynamically placing the heap in memory so we can dynamically change the size.
| * | ||
| * The MIT License (MIT) | ||
| * | ||
| * Copyright (c) 2016 Scott Shawcroft for Adafruit Industries |
There was a problem hiding this comment.
You may want to add yourself to some of these copyrights. :-)
|
|
||
| // Wait for TX complete | ||
| if ( len ) | ||
| { |
There was a problem hiding this comment.
FYI my style is usually if (len) {. I won't nitpick it though until we can auto-validate it.
| } | ||
|
|
||
| void common_hal_pulseio_pulsein_construct(pulseio_pulsein_obj_t* self, const mcu_pin_obj_t* pin, uint16_t maxlen, bool idle_state) { | ||
|
|
There was a problem hiding this comment.
I think you can mp_raise_NotImplementedError(NULL); here to throw an exception indicating its not implemented. Null can be a string if you want to explain why its not implemented.
| } | ||
|
|
||
|
|
||
| #if 0 |
There was a problem hiding this comment.
I'd suggest removing this disabled code because it is already available in another port.
| } | ||
|
|
||
| void common_hal_pulseio_pulseout_construct(pulseio_pulseout_obj_t* self, const pulseio_pwmout_obj_t* carrier) { | ||
|
|
There was a problem hiding this comment.
Same thing here, raise NotImplementedError and remove duplicated code.
|
The zip file issue might be due to git's auto line-ending stuff thinking its a text file. |
|
FYI - I was able to merge the latest version into master and create/upload a working version for my feather52 board. |
|
One other thing to do in this PR or a follow up would be getting Travis to build the feather52 binary every commit. That way we'll know it always builds. :-) To do that |
|
@microbuilder I think that only runs it off Travis builds. The ESP8266 isn't built on travis because the toolchain isn't available on Travis. You'll want to add a TRAVIS_BOARD here for it: https://github.com/adafruit/circuitpython/blob/master/.travis.yml#L15 That list makes the checks run in parallel and speeds up the build. |
tannewt
left a comment
There was a problem hiding this comment.
One additional comment. Its ready to go in once Travis is building it.
| { MP_ROM_QSTR(MP_QSTR_A5 ), MP_ROM_PTR(&pin_PA29) }, | ||
| { MP_ROM_QSTR(MP_QSTR_A6 ), MP_ROM_PTR(&pin_PA30) }, | ||
| { MP_ROM_QSTR(MP_QSTR_A7 ), MP_ROM_PTR(&pin_PA31) }, | ||
| { MP_ROM_QSTR(MP_QSTR_D13 ), MP_ROM_PTR(&pin_PA17) }, // LED for stdrd exmpl |
There was a problem hiding this comment.
// LED for standard examples Abbreviated versions like this makes it harder to read.
There was a problem hiding this comment.
It's always a tradeoff since wanting to stick to 80 chars wide, and abbreviation, but if you prefer to run over I'll update this.
There was a problem hiding this comment.
Running over or placing the comment above is fine.
|
@tannewt This includes 'feather52' builds on travis now, but the CTRL+C issue with REPL still needs to be addressed before this can be merged (hopefully tomorrow). |
|
@jerryneedell CRLT+C breaks should work in REPL now! |
|
Thank you ! I have updated and successfully tested the control - C. |
|
@tannewt I think this can be merged in as is, but let me know if there is something I missed? |
tannewt
left a comment
There was a problem hiding this comment.
This is awesome! I'm super excited for this. I tweak the travis file a little bit so the nrf52 download only happens when building the nrf52. Will merge it after Travis finishes.
Added support for internal file system, and numerous 3.x series changes to bring MCU support up to date with master.
NOTE: The following addition was required to
supervisor/port.h filefor the nRF52:Everything else is contained in the port folder.
For test and build instructions see
ports/nrf/boards/feather52/README.md