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

Nordic port for nrf51822, nrf52832 and nrf52840 #3137

Closed
wants to merge 945 commits into
base: master
from

Conversation

Projects
None yet
@tralamazza
Contributor

tralamazza commented Jun 9, 2017

This is an initial port for the nRF series including nrf51822, nrf52832 and nrf52840 support.

Modules currently supported:

  • UART
  • LED
  • Pin
  • ADC
  • PWM (nrf52)
  • SPI
  • I2C

Additional micropython modules:

  • music - backport of micro:bit music module for both nrf51 and nrf52 (+ ticker and softpwm drivers)
  • Temperature
  • RTC (Real time counter - low-power counter)
  • ubluepy - Bluetooth LE module using existing python library, bluepy, as API template. (Extended to also act as a peripheral role)

Additional features:

  • Bluetooth LE REPL

Current target boards supported by nrf51822:

  • BBC micro:bit
  • pca10000 (dongle)
  • pca10001 (dev-kit)
  • pca10028 (dev-kit)
  • pca10032 (dongle)

Current target boards supported by nrf52832:

  • Bluefruit nRF52 Feather
  • BL652 & DVK from Laird
  • pca10040

Current target board supported by nrf52840: pca10056

The port introduces a new HAL written from scratch to make it license compatible with micropython for redistribution. Header files for individual chips (Nordic Semi MDK) are distributed, as it uses a compatible BSD license. Hence, no dependency on any other 3rd party for HAL or libraries.
All targets, except for the nrf52840, have Bluetooth LE support using Nordic Semiconductor Bluetooth Stacks (Softdevice) downloaded directly from Nordic's website using the provided script.

Some modules are not be 100% aligned with current micropython machine API. However, the port is functional and gives a good base to start writing micropython code on the nrf5x devices with or without Bluetooth LE.

Subsequent patches are expected to get the port up to date to the new machine module APIs, adding new HAL drivers and modules, adding new target boards and optimizing flash/ram footprint (specially on the nrf51 targets).

glennrub added some commits May 14, 2017

nrf5/drivers/bluetooth: Renaming attr_write and attr_notify to attr_s…
…_write and attr_s_notify to prepare for introduction of attribute write for gatt client.
nrf5/modules/ubluepy: Updating characteristic object write function t…
…o be role aware. Either peripheral or central (gatts or gattc). Adding dummy call to attr_c_write if central is compiled in. Still in progress to be implemented.
Powerup (#26)
* nrf5/examples: Adding python example template for PowerUp 3.0 Bluetooth LE controlled Paper Airplane.

* nrf5: Enable bluetooth le central while developing powerup 3.0 example.

* nrf5/examples: Backing up powerup 3.0 progress.

* nrf5/examples: Adding working example on how to control PowerUp 3.0 paper airplane using bluetooth le.

* nrf5/bluetooth: Disable central role.
nrf5/examples: Fixing overlapping function names and variable names i…
…nside the object. Also removing some print statements. Tuning max angle from -7/7 to -25/25.
nrf5/drivers/bluetooth: As callback functions are in most usecases ar…
…e set to NULL upon last event to get public API function out of blocking mode, these function pointers has to be set as volatile, as they are updated to NULL in interrupt context, but read in blocking main-thread.
nrf5/modules/ubluepy: Making peripheral conn_handle volatile. Upon co…
…nnection event, the variable is accessed in thread mode. However, the main-loop is blocking on conn_handle != 0xFFFF. If this is not volatile, optimized code will not exit the loop.
nrf5/modules/music: Update modmusic to use updated includes. Add exte…
…rn ticks. Add function which implements initialization of pwm and ticker, register ticker callback, and start the pwm and ticker. This corresponds to microbit port main.cpp init.
nrf5: Update Makefile to include ticker.c and renamed softpwm. Updati…
…ng also include paths to include modules/music and drivers/.
nrf5/drivers/softpwm: Renaming pwm_init to softpwm_init to not collid…
…e on symbol name with pwm_init in nrf52 machine PWM object.
nrf5/drivers/ticker: Add compile config guard in ticker.c to only inc…
…lude the driver if SOFT_PWM is configured in by board.
nrf5/hal/timer: Quickfix. Disable IRQ handler if SOFT_PWM is configur…
…ed to be enabled. Ticker driver has in current driver a seperate IRQ handler for this timer instance.
nrf5/modules/machine: Quickfix. Update timer object to not allow inst…
…anciation of Timer(0) if SOFT_PWM is enabled by board.
nrf5/boards/microbit: Enable music module by default. However, timer …
…and rtc module has to be disabled. Bluetooth support broken. Optimization needed.
nrf5: Facilitate option to configure away the modble if needed. Enabl…
…ed if MICROPY_PY_BLE config is enabled in bluetooth_conf.h.
nrf5/drivers/ticker: Removing LowPriority callback from nrf51 as ther…
…e is only one SoftwareIRQ free if bluetooth stack is enabled. Also setting new IRQ priority on SlowTicker to 3 instead of 2, to interleave with bluetooth stack if needed. Updating all NVIC calls to use hal_irq.h defined static inlines instead of direct access.
nrf5/hal/irq: Adding include of nrf_nvic.h if s132 bluetooth stack is…
… used to resolve IRQ function wrappers on newer bluetooth stacks.
nrf5/examples: Tuning Bluetooth LE example controller python script a…
…fter testing out the example live. Motor speed of 100 was not enought to lift the airplane. Also turning was hard without setting higher angle values. The new values are just guessed values. However, the flying experience was good.

glennrub added some commits Jun 19, 2018

nrf: Add explicit make flag for oofatfs
Adding MICROPY_FATFS as makefile flag in order to explicitly
include oofatfs files to be compiled into the build.

The flag is set to 0 by default. Must be set in addition to
MICROPY_VFS and MICROPY_VFS_FAT in mpconfigport.h.
nrf: Compile nlr objects with -fno-lto flag
To prevent over-optimizations of nlr and nlrthumb when -flto is used
the flag -fno-lto is set on these modules during compilation.
@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 3, 2018

Contributor

@vshymanskyy, representing this PR/nrf-port i do not see any blockers. @dpgeorge, could you comment on plans of getting it merged, or whether you see any blockers?

As stated before, we are ready to move the development and maintenance to mainline as soon as/if merged.

Contributor

glennrub commented Jul 3, 2018

@vshymanskyy, representing this PR/nrf-port i do not see any blockers. @dpgeorge, could you comment on plans of getting it merged, or whether you see any blockers?

As stated before, we are ready to move the development and maintenance to mainline as soon as/if merged.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 4, 2018

Contributor

@dpgeorge, could you comment on plans of getting it merged, or whether you see any blockers?

As stated before, we are ready to move the development and maintenance to mainline as soon as/if merged.

Apologies for not keeping up with this PR. It's mainly because there is so much other work to do just to maintain and develop everything else :) But I can see that this has been actively maintained for over a year now so am confident to add it to mainline as an officially supported port.

So, I just need to think what's the best way to proceed. Following the usual practice of commits here, this PR would be rebased on top of master. I don't know if it makes sense to squash a few/many/all of the commits, or none. And maybe some support libraries should be put into a submodule.

Contributor

dpgeorge commented Jul 4, 2018

@dpgeorge, could you comment on plans of getting it merged, or whether you see any blockers?

As stated before, we are ready to move the development and maintenance to mainline as soon as/if merged.

Apologies for not keeping up with this PR. It's mainly because there is so much other work to do just to maintain and develop everything else :) But I can see that this has been actively maintained for over a year now so am confident to add it to mainline as an officially supported port.

So, I just need to think what's the best way to proceed. Following the usual practice of commits here, this PR would be rebased on top of master. I don't know if it makes sense to squash a few/many/all of the commits, or none. And maybe some support libraries should be put into a submodule.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 4, 2018

Contributor

Also, licensing and copyright for everything here needs to be checked.

Contributor

dpgeorge commented Jul 4, 2018

Also, licensing and copyright for everything here needs to be checked.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 4, 2018

Contributor

Rebasing on top of master goes very smoothly, thanks for keeping it clean! There are 901 commits so it would be good to squash that down somehow. It seems that a lot of the work was done before the move to the ports/ directory, so perhaps it makes sense to squash all the commits before the move, and that would squash about 800 commits into one...

Contributor

dpgeorge commented Jul 4, 2018

Rebasing on top of master goes very smoothly, thanks for keeping it clean! There are 901 commits so it would be good to squash that down somehow. It seems that a lot of the work was done before the move to the ports/ directory, so perhaps it makes sense to squash all the commits before the move, and that would squash about 800 commits into one...

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 4, 2018

Contributor

And maybe some support libraries should be put into a submodule.

I see that the nrfx HAL code is already a submodule, great!

And building a few of the boards works perfectly (using arm-none-eabi-gcc 8.1.0).

I tested the microbit board and it works, except it hangs on soft reset (ctrl-D). But that's not a blocker to merge this PR.

Contributor

dpgeorge commented Jul 4, 2018

And maybe some support libraries should be put into a submodule.

I see that the nrfx HAL code is already a submodule, great!

And building a few of the boards works perfectly (using arm-none-eabi-gcc 8.1.0).

I tested the microbit board and it works, except it hangs on soft reset (ctrl-D). But that's not a blocker to merge this PR.

@tannewt

This comment has been minimized.

Show comment
Hide comment
@tannewt

tannewt Jul 4, 2018

tannewt commented Jul 4, 2018

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 4, 2018

Contributor

I see that there's a custom random module, which shouldn't be necessary. In fact it looks like it's using HW RNG and I think this is wrong because the random module should use a PRNG so that the random sequence is reproducible by setting the seed. There's always os.urandom for true random numbers.

Contributor

dpgeorge commented Jul 4, 2018

I see that there's a custom random module, which shouldn't be necessary. In fact it looks like it's using HW RNG and I think this is wrong because the random module should use a PRNG so that the random sequence is reproducible by setting the seed. There's always os.urandom for true random numbers.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 4, 2018

Contributor

There's also the addition of microbit code: display, images and music. In principle it's ok to have this, and the display code is useful. Ultimately it would be good to support the microbit via this repository, but getting 100% compatibility with the existing microbit API will require a fair bit more effort, and merging of more code from there (eg the speech module, with its license). Well, that's not really to decide right now, just to think about whether something like the music code should be included.

Contributor

dpgeorge commented Jul 4, 2018

There's also the addition of microbit code: display, images and music. In principle it's ok to have this, and the display code is useful. Ultimately it would be good to support the microbit via this repository, but getting 100% compatibility with the existing microbit API will require a fair bit more effort, and merging of more code from there (eg the speech module, with its license). Well, that's not really to decide right now, just to think about whether something like the music code should be included.

@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 4, 2018

Contributor

So, I just need to think what's the best way to proceed. Following the usual practice of commits here, this PR would be rebased on top of master. I don't know if it makes sense to squash a few/many/all of the commits, or none. And maybe some support libraries should be put into a submodule.

Thanks @dpgeorge for the feedback, and i'm happy to hear that the PR is considered to be merged!

I would imagine that a squash of all of the commits would be OK. However, as @tannewt pointed out it might get a bit tricky for ciruitpython if all commit ids change. I'm not an expert on what would be the best here.

Also, licensing and copyright for everything here needs to be checked.

I believe licenses/copyright's should be pretty much in order. It would of course be good that it's checked explicitly. Is there a tool for this? or would it be manually checking?

A note on microbit files:
For the microbit ported files under boards/microbit/modules the license and author's file has been supplied as the license text of that repository describes it. (bbcmicrobit/micropython#460)
Other modules from microbit that has been ported; modrandom, modmusic, microbitfs, softpwm and ticker includes license text in-file which grants permission to copy/modify/redistribute. Hence, moved out and detached from the microbit/modules license.

I see that there's a custom random module, which shouldn't be necessary. In fact it looks like it's using HW RNG and I think this is wrong because the random module should use a PRNG so that the random sequence is reproducible by setting the seed. There's always os.urandom for true random numbers.

We'll take a look at this, shouldn't be too hard to remove the HW_RNG one.

There's also the addition of microbit code: display, images and music. In principle it's ok to have this, and the display code is useful. Ultimately it would be good to support the microbit via this repository, but getting 100% compatibility with the existing microbit API will require a fair bit more effort, and merging of more code from there (eg the speech module, with its license). Well, that's not really to decide right now, just to think about whether something like the music code should be included.

Yes, i agree, the display module is almost a must for microbit to make something fun. Additional drivers/modules ported from microbit (modmusic, modrandom, microbit-FS, softpwm and ticker) has been adopted to additionally run on nrf52, and run simultaneously with Bluetooth LE. So i guess from my side it's not only about supporting the microbit from this port, but also to share the good drivers written for microbit with other compile targets.

Contributor

glennrub commented Jul 4, 2018

So, I just need to think what's the best way to proceed. Following the usual practice of commits here, this PR would be rebased on top of master. I don't know if it makes sense to squash a few/many/all of the commits, or none. And maybe some support libraries should be put into a submodule.

Thanks @dpgeorge for the feedback, and i'm happy to hear that the PR is considered to be merged!

I would imagine that a squash of all of the commits would be OK. However, as @tannewt pointed out it might get a bit tricky for ciruitpython if all commit ids change. I'm not an expert on what would be the best here.

Also, licensing and copyright for everything here needs to be checked.

I believe licenses/copyright's should be pretty much in order. It would of course be good that it's checked explicitly. Is there a tool for this? or would it be manually checking?

A note on microbit files:
For the microbit ported files under boards/microbit/modules the license and author's file has been supplied as the license text of that repository describes it. (bbcmicrobit/micropython#460)
Other modules from microbit that has been ported; modrandom, modmusic, microbitfs, softpwm and ticker includes license text in-file which grants permission to copy/modify/redistribute. Hence, moved out and detached from the microbit/modules license.

I see that there's a custom random module, which shouldn't be necessary. In fact it looks like it's using HW RNG and I think this is wrong because the random module should use a PRNG so that the random sequence is reproducible by setting the seed. There's always os.urandom for true random numbers.

We'll take a look at this, shouldn't be too hard to remove the HW_RNG one.

There's also the addition of microbit code: display, images and music. In principle it's ok to have this, and the display code is useful. Ultimately it would be good to support the microbit via this repository, but getting 100% compatibility with the existing microbit API will require a fair bit more effort, and merging of more code from there (eg the speech module, with its license). Well, that's not really to decide right now, just to think about whether something like the music code should be included.

Yes, i agree, the display module is almost a must for microbit to make something fun. Additional drivers/modules ported from microbit (modmusic, modrandom, microbit-FS, softpwm and ticker) has been adopted to additionally run on nrf52, and run simultaneously with Bluetooth LE. So i guess from my side it's not only about supporting the microbit from this port, but also to share the good drivers written for microbit with other compile targets.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 5, 2018

Contributor

Merging rather than rebasing would be best for circuitpython since we’ve already merged it in. Otherwise all the commit ids will change and be duplicated.

@tannewt I think it might actually be cleaner (a cleaner git history) for you if I do a rebase and squash a lot of commits. If I do a pure merge, then when you merge us you will see a big merge of this nrf branch, plus connections in the graph to all the commits you already merged. And then you need to merge the merge I make. On the other hand, if I do a rebase and squash then you'll see a handful of new commits for the nrf port which will just be like any normal commit we make in this repo, and you then just have to merge them as usual. But because you already have the changes in the rebased/squashed commits the merge will be blank (given that there were no changes in the meantime). And the history will be much cleaner for you. All your original merges of the nrf code will still be in your repo. Maybe you want to give this a test run, try out both scenarios, to see exactly how it works?

Contributor

dpgeorge commented Jul 5, 2018

Merging rather than rebasing would be best for circuitpython since we’ve already merged it in. Otherwise all the commit ids will change and be duplicated.

@tannewt I think it might actually be cleaner (a cleaner git history) for you if I do a rebase and squash a lot of commits. If I do a pure merge, then when you merge us you will see a big merge of this nrf branch, plus connections in the graph to all the commits you already merged. And then you need to merge the merge I make. On the other hand, if I do a rebase and squash then you'll see a handful of new commits for the nrf port which will just be like any normal commit we make in this repo, and you then just have to merge them as usual. But because you already have the changes in the rebased/squashed commits the merge will be blank (given that there were no changes in the meantime). And the history will be much cleaner for you. All your original merges of the nrf code will still be in your repo. Maybe you want to give this a test run, try out both scenarios, to see exactly how it works?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 5, 2018

Contributor

I would imagine that a squash of all of the commits would be OK.

@glennrub if you (and other authors on this branch) are OK with a rebase and squash (maybe not of all, but at least of all commits before the move to the ports/ dir) then that is definitely my preferred option.

I believe licenses/copyright's should be pretty much in order. It would of course be good that it's checked explicitly. Is there a tool for this? or would it be manually checking?

I'll go through and check it manually. I think it's the only way to do it properly.

We'll take a look at this, shouldn't be too hard to remove the HW_RNG one.

Ok. It's not a blocker to merge this, we can do the change after a merged. I was more curious as to why it was duplicated in this way.

So i guess from my side it's not only about supporting the microbit from this port, but also to share the good drivers written for microbit with other compile targets.

Yes that's a good goal, to have the microbit classes available for other ports. That can come it time as well.

So I guess the things to do to get this merged are:

  1. decide/agree that rebasing and squashing is the right way to merge the code
  2. check licensing and copyright
Contributor

dpgeorge commented Jul 5, 2018

I would imagine that a squash of all of the commits would be OK.

@glennrub if you (and other authors on this branch) are OK with a rebase and squash (maybe not of all, but at least of all commits before the move to the ports/ dir) then that is definitely my preferred option.

I believe licenses/copyright's should be pretty much in order. It would of course be good that it's checked explicitly. Is there a tool for this? or would it be manually checking?

I'll go through and check it manually. I think it's the only way to do it properly.

We'll take a look at this, shouldn't be too hard to remove the HW_RNG one.

Ok. It's not a blocker to merge this, we can do the change after a merged. I was more curious as to why it was duplicated in this way.

So i guess from my side it's not only about supporting the microbit from this port, but also to share the good drivers written for microbit with other compile targets.

Yes that's a good goal, to have the microbit classes available for other ports. That can come it time as well.

So I guess the things to do to get this merged are:

  1. decide/agree that rebasing and squashing is the right way to merge the code
  2. check licensing and copyright
@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 5, 2018

Contributor

glennrub if you (and other authors on this branch) are OK with a rebase and squash (maybe not of all, but at least of all commits before the move to the ports/ dir) then that is definitely my preferred option.

@dpgeorge, i've reached out to the other co-authors to ask if a big squash is acceptable, and the fact that credit/commit history gets lost. I thinks as a minimum all co-authors should get credited in the big squash commit.

Contributor

glennrub commented Jul 5, 2018

glennrub if you (and other authors on this branch) are OK with a rebase and squash (maybe not of all, but at least of all commits before the move to the ports/ dir) then that is definitely my preferred option.

@dpgeorge, i've reached out to the other co-authors to ask if a big squash is acceptable, and the fact that credit/commit history gets lost. I thinks as a minimum all co-authors should get credited in the big squash commit.

@aykevl

This comment has been minimized.

Show comment
Hide comment
@aykevl

aykevl Jul 5, 2018

Contributor

Great to see this PR so close to a merge!

@glennrub if you (and other authors on this branch) are OK with a rebase and squash (maybe not of all, but at least of all commits before the move to the ports/ dir) then that is definitely my preferred option.

I'm generally no fan of rewriting/squashing history (except for single-feature PRs) but if you'd rather see older commits squashed that's fine to me. I would, however, prefer when the more recent commits (e.g. after the ports/ move) wouldn't be squashed. It breaks git blame among others.

Something else: I'm wondering how this port will be managed after the merge? Lately it's mostly been managed by @glennrub with me as an on-and-off active contributor with PRs being closed fairly quickly, but I'm worried that development will slow down after the merge, as you already seem to have a lot of work with MicroPython. How will management be organized after the merge? I can think of a few ways:

  • Business as usual - you review and merge patches.
  • More like the Linux development model, e.g. review is mostly done by one of the port maintainers but the final merge is done by you. Pre-reviewed PRs don't need an extensive review so can be merged more quickly.
  • Give port maintainers commit access so they can review and merge PRs themselves (but only for their port).
Contributor

aykevl commented Jul 5, 2018

Great to see this PR so close to a merge!

@glennrub if you (and other authors on this branch) are OK with a rebase and squash (maybe not of all, but at least of all commits before the move to the ports/ dir) then that is definitely my preferred option.

I'm generally no fan of rewriting/squashing history (except for single-feature PRs) but if you'd rather see older commits squashed that's fine to me. I would, however, prefer when the more recent commits (e.g. after the ports/ move) wouldn't be squashed. It breaks git blame among others.

Something else: I'm wondering how this port will be managed after the merge? Lately it's mostly been managed by @glennrub with me as an on-and-off active contributor with PRs being closed fairly quickly, but I'm worried that development will slow down after the merge, as you already seem to have a lot of work with MicroPython. How will management be organized after the merge? I can think of a few ways:

  • Business as usual - you review and merge patches.
  • More like the Linux development model, e.g. review is mostly done by one of the port maintainers but the final merge is done by you. Pre-reviewed PRs don't need an extensive review so can be merged more quickly.
  • Give port maintainers commit access so they can review and merge PRs themselves (but only for their port).
@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 6, 2018

Contributor

I thinks as a minimum all co-authors should get credited in the big squash commit.

Yes, definitely we will do this. See how it was done for the esp32 port here: bc08c88

I would, however, prefer when the more recent commits (e.g. after the ports/ move) wouldn't be squashed.

Ok, I think we can accommodate this.

I'm wondering how this port will be managed after the merge?

I'm happy to give push access to nrf maintainer(s) to make management more efficient. The idea would be for a process like this:

  • minor changes to the nrf port could be pushed by nrf maintainer(s) without review (but the changes should only touch code in the ports/nrf dir, or associated location in docs/)
  • major changes to the nrf port, or uncertain changes, would ideally be put up for review
  • changes that touch other parts of the code base must go up for review
  • PRs/contributions to nrf port can be merged by nrf maintainers

If you have other ideas/suggestions, let me know.

Contributor

dpgeorge commented Jul 6, 2018

I thinks as a minimum all co-authors should get credited in the big squash commit.

Yes, definitely we will do this. See how it was done for the esp32 port here: bc08c88

I would, however, prefer when the more recent commits (e.g. after the ports/ move) wouldn't be squashed.

Ok, I think we can accommodate this.

I'm wondering how this port will be managed after the merge?

I'm happy to give push access to nrf maintainer(s) to make management more efficient. The idea would be for a process like this:

  • minor changes to the nrf port could be pushed by nrf maintainer(s) without review (but the changes should only touch code in the ports/nrf dir, or associated location in docs/)
  • major changes to the nrf port, or uncertain changes, would ideally be put up for review
  • changes that touch other parts of the code base must go up for review
  • PRs/contributions to nrf port can be merged by nrf maintainers

If you have other ideas/suggestions, let me know.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 6, 2018

Contributor

I did a test-run of rebasing and squashing all the commits (around 800) before the move to the ports directory, and squashed them with the commit that did the move of nrf5/ to ports/nrf. You can see the branch here: https://github.com/dpgeorge/micropython/commits/ports-add-nrf . In particular the first, squashed commit, with a very long commit message (that might be made a bit shorter): dpgeorge@280afa8 . I left the other 97 commits, after the move to ports/nrf/, unsquashed (but rebased, and there were some edits to the commit messages to make them more consistent with the nrf prefix at the start).

It's either this aproach, or no squashing at all and just rebasing the 900 or-so commits (and trying to reword the commit messages to not run over 72 characters on the first line...).

Contributor

dpgeorge commented Jul 6, 2018

I did a test-run of rebasing and squashing all the commits (around 800) before the move to the ports directory, and squashed them with the commit that did the move of nrf5/ to ports/nrf. You can see the branch here: https://github.com/dpgeorge/micropython/commits/ports-add-nrf . In particular the first, squashed commit, with a very long commit message (that might be made a bit shorter): dpgeorge@280afa8 . I left the other 97 commits, after the move to ports/nrf/, unsquashed (but rebased, and there were some edits to the commit messages to make them more consistent with the nrf prefix at the start).

It's either this aproach, or no squashing at all and just rebasing the 900 or-so commits (and trying to reword the commit messages to not run over 72 characters on the first line...).

@aykevl

This comment has been minimized.

Show comment
Hide comment
@aykevl

aykevl Jul 6, 2018

Contributor

I took a quick look at the proposed PR and it looks good!

Just a note: the ubluepy module in there is not considered stable as we haven't agreed on a BLE API yet. So we shouldn't promise backwards compatibility to it, it may be replaced some day. I think most other things are roughly stable.

I'm happy to give push access to nrf maintainer(s) to make management more efficient. The idea would be for a process like this: [...]

Sounds good!

Contributor

aykevl commented Jul 6, 2018

I took a quick look at the proposed PR and it looks good!

Just a note: the ubluepy module in there is not considered stable as we haven't agreed on a BLE API yet. So we shouldn't promise backwards compatibility to it, it may be replaced some day. I think most other things are roughly stable.

I'm happy to give push access to nrf maintainer(s) to make management more efficient. The idea would be for a process like this: [...]

Sounds good!

@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 6, 2018

Contributor

I did a test-run of rebasing and squashing all the commits (around 800) before the move to the ports directory, and squashed them with the commit that did the move of nrf5/ to ports/nrf. You can see the branch here: https://github.com/dpgeorge/micropython/commits/ports-add-nrf . In particular the first, squashed commit, with a very long commit message (that might be made a bit shorter):

@dpgeorge , Thanks a lot for doing this test-run to see how it could look like! For the squashed commit, we can probably replace the commit log there with either the names on the contributors that has contributed with commits to this squash set or trim down the log by removing my entries, keeping the other contributors only. If wanted, i can provide the hashes of these commits, or name/usernames on the contributors which is part of the squashed commit.

I have received positive feedback from the co-authors that they are OK with squashing commits. @aykevl's comment about keeping the recent commits as is after the move to ports/ was a good input, and i agree that it would make sense to do it that way. And your test-run looks like it is addressing this very nicely.

Summarized, the only modification to the test-run which i would like is to update the commit message to credit the other co-authors of their commits in the squash. Else, it looks good from my side.

Contributor

glennrub commented Jul 6, 2018

I did a test-run of rebasing and squashing all the commits (around 800) before the move to the ports directory, and squashed them with the commit that did the move of nrf5/ to ports/nrf. You can see the branch here: https://github.com/dpgeorge/micropython/commits/ports-add-nrf . In particular the first, squashed commit, with a very long commit message (that might be made a bit shorter):

@dpgeorge , Thanks a lot for doing this test-run to see how it could look like! For the squashed commit, we can probably replace the commit log there with either the names on the contributors that has contributed with commits to this squash set or trim down the log by removing my entries, keeping the other contributors only. If wanted, i can provide the hashes of these commits, or name/usernames on the contributors which is part of the squashed commit.

I have received positive feedback from the co-authors that they are OK with squashing commits. @aykevl's comment about keeping the recent commits as is after the move to ports/ was a good input, and i agree that it would make sense to do it that way. And your test-run looks like it is addressing this very nicely.

Summarized, the only modification to the test-run which i would like is to update the commit message to credit the other co-authors of their commits in the squash. Else, it looks good from my side.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 10, 2018

Contributor

Just a note: the ubluepy module in there is not considered stable as we haven't agreed on a BLE API yet. So we shouldn't promise backwards compatibility to it, it may be replaced some day. I think most other things are roughly stable.

Ok, noted.

Thanks a lot for doing this test-run to see how it could look like

I've now updated this branch (at https://github.com/dpgeorge/micropython/commits/ports-add-nrf) (with a force-push) to make all the commit messages conform to the format used in this repo. I tried to make minimal modifications when doing it. @glennrub you may want to have a quick look to check that you're happy with how it looks now. There are still the 98 commits in total which retain the history of all the changes since Oct 2017 (9 months).

For the squashed commit, we can probably replace the commit log there with either the names on the contributors that has contributed with commits to this squash set or trim down the log by removing my entries, keeping the other contributors only. If wanted, i can provide the hashes of these commits, or name/usernames on the contributors which is part of the squashed commit.

Retaining the hashes of the commits (in the long commit message) only make sense if they point to somewhere that can last indefinitely. Eg if the original history is preserved somewhere in a repo (eg yours, on a branch) so that the hashes remain constant.

To make the commit message more readable but still retain most of the history and credit, you may want to do it like was done with the merge of the ESP32 code, see commit here: bc08c88 . To make this I just combined adjacent commits by the same author in one paragraph, and listed the author's name just once at the start of each paragraph.

Contributor

dpgeorge commented Jul 10, 2018

Just a note: the ubluepy module in there is not considered stable as we haven't agreed on a BLE API yet. So we shouldn't promise backwards compatibility to it, it may be replaced some day. I think most other things are roughly stable.

Ok, noted.

Thanks a lot for doing this test-run to see how it could look like

I've now updated this branch (at https://github.com/dpgeorge/micropython/commits/ports-add-nrf) (with a force-push) to make all the commit messages conform to the format used in this repo. I tried to make minimal modifications when doing it. @glennrub you may want to have a quick look to check that you're happy with how it looks now. There are still the 98 commits in total which retain the history of all the changes since Oct 2017 (9 months).

For the squashed commit, we can probably replace the commit log there with either the names on the contributors that has contributed with commits to this squash set or trim down the log by removing my entries, keeping the other contributors only. If wanted, i can provide the hashes of these commits, or name/usernames on the contributors which is part of the squashed commit.

Retaining the hashes of the commits (in the long commit message) only make sense if they point to somewhere that can last indefinitely. Eg if the original history is preserved somewhere in a repo (eg yours, on a branch) so that the hashes remain constant.

To make the commit message more readable but still retain most of the history and credit, you may want to do it like was done with the merge of the ESP32 code, see commit here: bc08c88 . To make this I just combined adjacent commits by the same author in one paragraph, and listed the author's name just once at the start of each paragraph.

@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 10, 2018

Contributor

I've now updated this branch (at https://github.com/dpgeorge/micropython/commits/ports-add-nrf) (with a force-push) to make all the commit messages conform to the format used in this repo. I tried to make minimal modifications when doing it. @glennrub you may want to have a quick look to check that you're happy with how it looks now. There are still the 98 commits in total which retain the history of all the changes since Oct 2017 (9 months).

@dpgeorge, the updated branch looks good to me!

To make the commit message more readable but still retain most of the history and credit, you may want to do it like was done with the merge of the ESP32 code, see commit here: bc08c88 . To make this I just combined adjacent commits by the same author in one paragraph, and listed the author's name just once at the start of each paragraph.

Great idea. If you need some help please let me know. Or, maybe you already have a script to automate this conversion a bit?

Contributor

glennrub commented Jul 10, 2018

I've now updated this branch (at https://github.com/dpgeorge/micropython/commits/ports-add-nrf) (with a force-push) to make all the commit messages conform to the format used in this repo. I tried to make minimal modifications when doing it. @glennrub you may want to have a quick look to check that you're happy with how it looks now. There are still the 98 commits in total which retain the history of all the changes since Oct 2017 (9 months).

@dpgeorge, the updated branch looks good to me!

To make the commit message more readable but still retain most of the history and credit, you may want to do it like was done with the merge of the ESP32 code, see commit here: bc08c88 . To make this I just combined adjacent commits by the same author in one paragraph, and listed the author's name just once at the start of each paragraph.

Great idea. If you need some help please let me know. Or, maybe you already have a script to automate this conversion a bit?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 10, 2018

Contributor

Great idea. If you need some help please let me know. Or, maybe you already have a script to automate this conversion a bit?

No script, but it shouldn't be too hard with some vim macros. I'll give it a try.

Contributor

dpgeorge commented Jul 10, 2018

Great idea. If you need some help please let me know. Or, maybe you already have a script to automate this conversion a bit?

No script, but it shouldn't be too hard with some vim macros. I'll give it a try.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 12, 2018

Contributor

@glennrub I updated the commit message for the first nrf commit on my branch. I also separated out the change you made to lib/utils/pyexec.h to expose pyb_set_repl_info as public. I think it's getting very close now to being able to merge.

Contributor

dpgeorge commented Jul 12, 2018

@glennrub I updated the commit message for the first nrf commit on my branch. I also separated out the change you made to lib/utils/pyexec.h to expose pyb_set_repl_info as public. I think it's getting very close now to being able to merge.

@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 12, 2018

Contributor

I updated the commit message for the first nrf commit on my branch.

@dpgeorge Great! I looked over the first commit and it looks very good and structured. Would it also be possible to rewrite the glennrub author to my name? I'm not sure how it ended up with a username as author in the commits there. I guess i might have had a bad git settings on my PC.

I also separated out the change you made to lib/utils/pyexec.h to expose pyb_set_repl_info as public.

Yes, that makes sense. I would guess thats the only one commit that stretched out of the ports/nrf folder. The other one was to lib/netutils adding parsing of ipv6 addresses, but that was reverted before the move to ports/ as BLE ipv6/6lowpan was removed at some stage. So, that one should be OK now as it is squashed.

I think it's getting very close now to being able to merge.

From my side it looks good. I believe the only thing i spotted was the mistyped username instead of full name on some of my commits in the squash commit. You can decide whether you want to update this or not, but i'm fine with how the PR looks.

Contributor

glennrub commented Jul 12, 2018

I updated the commit message for the first nrf commit on my branch.

@dpgeorge Great! I looked over the first commit and it looks very good and structured. Would it also be possible to rewrite the glennrub author to my name? I'm not sure how it ended up with a username as author in the commits there. I guess i might have had a bad git settings on my PC.

I also separated out the change you made to lib/utils/pyexec.h to expose pyb_set_repl_info as public.

Yes, that makes sense. I would guess thats the only one commit that stretched out of the ports/nrf folder. The other one was to lib/netutils adding parsing of ipv6 addresses, but that was reverted before the move to ports/ as BLE ipv6/6lowpan was removed at some stage. So, that one should be OK now as it is squashed.

I think it's getting very close now to being able to merge.

From my side it looks good. I believe the only thing i spotted was the mistyped username instead of full name on some of my commits in the squash commit. You can decide whether you want to update this or not, but i'm fine with how the PR looks.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 12, 2018

Contributor

Would it also be possible to rewrite the glennrub author to my name?

No problem, now done.

The other one was to lib/netutils adding parsing of ipv6 addresses, but that was reverted

Yes I saw that, and this change has been cleanly removed.

Contributor

dpgeorge commented Jul 12, 2018

Would it also be possible to rewrite the glennrub author to my name?

No problem, now done.

The other one was to lib/netutils adding parsing of ipv6 addresses, but that was reverted

Yes I saw that, and this change has been cleanly removed.

@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 12, 2018

Contributor

No problem, now done.

Thank you, @dpgeorge! Now, it all looks good from my side.

Contributor

glennrub commented Jul 12, 2018

No problem, now done.

Thank you, @dpgeorge! Now, it all looks good from my side.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 16, 2018

Contributor

I've now checked the copyright and licensing and am happy with it. Thank you very much @glennrub and @aykevl and other contributors for keeping it clean.

I submitted PR #3948 which is this PR with the commits squashed as discussed above, and that would be the one that is merged instead of this PR (it's the same code of course!). @glennrub I did not make any changes to that branch of mine (which is now the PR) since you last looked at it (when I fixed your author name).

Contributor

dpgeorge commented Jul 16, 2018

I've now checked the copyright and licensing and am happy with it. Thank you very much @glennrub and @aykevl and other contributors for keeping it clean.

I submitted PR #3948 which is this PR with the commits squashed as discussed above, and that would be the one that is merged instead of this PR (it's the same code of course!). @glennrub I did not make any changes to that branch of mine (which is now the PR) since you last looked at it (when I fixed your author name).

@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 16, 2018

Contributor

@dpgeorge, The two headers,

  • ports/nrf/drivers/ticker.h
  • ports/nrf/drivers/softpwm.h

lacks license and copyright notice from bbcmicrobit/micropython. I would assume it would be bad practice to add license text and copyright on others behalf (Relates to the issue in bbcmicrobit/micropython: bbcmicrobit/micropython#460). Should we consider to copy the AUTHORS file and LICENSE from from boards/microbit/modules to ports/nrf/drivers?
Do you see any problems that these header files lacks copyright statement in the headers, or is it covered by the c-implementation?

Contributor

glennrub commented Jul 16, 2018

@dpgeorge, The two headers,

  • ports/nrf/drivers/ticker.h
  • ports/nrf/drivers/softpwm.h

lacks license and copyright notice from bbcmicrobit/micropython. I would assume it would be bad practice to add license text and copyright on others behalf (Relates to the issue in bbcmicrobit/micropython: bbcmicrobit/micropython#460). Should we consider to copy the AUTHORS file and LICENSE from from boards/microbit/modules to ports/nrf/drivers?
Do you see any problems that these header files lacks copyright statement in the headers, or is it covered by the c-implementation?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 17, 2018

Contributor

The two headers ... lacks license and copyright notice from bbcmicrobit/micropython.

Ok, so there's the issue of licensing and of copyright. For the former, the LICENSE file at the root of the micro:bit repo will cover these header files. The license says that "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software". Since these files are copied we should include the copyright and permission notice "in the Software". So I think it's fair to put the LICENSE data at the start of these header files.

For the copyright, we can see from the git history of the micro:bit repo that these two headers had Mark Shannon and myself as authors, but to be on the safe/fair side we should list all names in the AUTHORS file as copyright holders.

So we can add this text at the top of these two headers and that should cover it in the right way.

Do you see any problems that these header files lacks copyright statement in the headers, or is it covered by the c-implementation?

There's not really much to copyright, and the license/copyright in the corresponding .c files would probably be enough to cover the .h as well, but to be on the safe side we can do as I suggest above. Would you be happy with that?

Contributor

dpgeorge commented Jul 17, 2018

The two headers ... lacks license and copyright notice from bbcmicrobit/micropython.

Ok, so there's the issue of licensing and of copyright. For the former, the LICENSE file at the root of the micro:bit repo will cover these header files. The license says that "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software". Since these files are copied we should include the copyright and permission notice "in the Software". So I think it's fair to put the LICENSE data at the start of these header files.

For the copyright, we can see from the git history of the micro:bit repo that these two headers had Mark Shannon and myself as authors, but to be on the safe/fair side we should list all names in the AUTHORS file as copyright holders.

So we can add this text at the top of these two headers and that should cover it in the right way.

Do you see any problems that these header files lacks copyright statement in the headers, or is it covered by the c-implementation?

There's not really much to copyright, and the license/copyright in the corresponding .c files would probably be enough to cover the .h as well, but to be on the safe side we can do as I suggest above. Would you be happy with that?

@glennrub

This comment has been minimized.

Show comment
Hide comment
@glennrub

glennrub Jul 17, 2018

Contributor

There's not really much to copyright, and the license/copyright in the corresponding .c files would probably be enough to cover the .h as well, but to be on the safe side we can do as I suggest above. Would you be happy with that?

@dpgeorge , Sorry for bringing this up so late in the process. I honestly had forgotten about these two files and found this by coincidence when i looked over the final PR. I'm good with the proposal, that should cover it. I've created a PR to tralamazza/master, adding the license and authors as suggested above;
tralamazza#213

Contributor

glennrub commented Jul 17, 2018

There's not really much to copyright, and the license/copyright in the corresponding .c files would probably be enough to cover the .h as well, but to be on the safe side we can do as I suggest above. Would you be happy with that?

@dpgeorge , Sorry for bringing this up so late in the process. I honestly had forgotten about these two files and found this by coincidence when i looked over the final PR. I'm good with the proposal, that should cover it. I've created a PR to tralamazza/master, adding the license and authors as suggested above;
tralamazza#213

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 18, 2018

Contributor

This has finally been merged, via #3948.

Thank you to all involved!

Contributor

dpgeorge commented Jul 18, 2018

This has finally been merged, via #3948.

Thank you to all involved!

@dpgeorge dpgeorge closed this Jul 18, 2018

@pfalcon

This comment has been minimized.

Show comment
Hide comment
@pfalcon

pfalcon Jul 18, 2018

Member

Even with 800-commit squash, this merge is still a big noise-bomb for the codebase. For example 7c74b7d, "nrf/drivers/softpwm: Rename init function to softpwm_init0.". Rename, why? If no reason, then why it's not squashed into the preceding commit touching that file? It's not the single case, it's fair to say that if 800 commits were squashed together, than a hundred more could have been squashed too. With extra pass to fix codestyle deviations.

Member

pfalcon commented Jul 18, 2018

Even with 800-commit squash, this merge is still a big noise-bomb for the codebase. For example 7c74b7d, "nrf/drivers/softpwm: Rename init function to softpwm_init0.". Rename, why? If no reason, then why it's not squashed into the preceding commit touching that file? It's not the single case, it's fair to say that if 800 commits were squashed together, than a hundred more could have been squashed too. With extra pass to fix codestyle deviations.

@aykevl

This comment has been minimized.

Show comment
Hide comment
@aykevl

aykevl Jul 18, 2018

Contributor

What's exactly the problem with these commits? Isn't that what git is supposed to do, handle commits (or rather: content)?
I personally have no problem with it at all, in fact I would have preferred even more history being kept (but that was a bit inconvenient). It makes it a lot easier to see where all these changes come from. Squashing breaks git blame.

Contributor

aykevl commented Jul 18, 2018

What's exactly the problem with these commits? Isn't that what git is supposed to do, handle commits (or rather: content)?
I personally have no problem with it at all, in fact I would have preferred even more history being kept (but that was a bit inconvenient). It makes it a lot easier to see where all these changes come from. Squashing breaks git blame.

@pfalcon

This comment has been minimized.

Show comment
Hide comment
@pfalcon

pfalcon Jul 18, 2018

Member

I would have preferred even more history being kept

I'm all for keeping more history. But the history should be clean. It should help answer questions, not confuse someone seeking for answers. I gave an example with 7c74b7d - the commit message doesn't explain why the change is made, so for someone exploring history of ports/nrf/drivers/softpwm.c file, that commit will be just noise. It should have been squashed into the commit which named that function softpwm_init(), so it came out with the right name, softpwm_init0(), in the history right away.

Of course, that's a minor case, but hundreds of such minor cases make history exploration incomprehensible. And work on any development branch (and any port, until it's merged, is a development branch) involves squashing and rebasing to maintain history clean. That's how @dpgeorge works on the branches, and how I worked on them too (and keep working in my fork). Of course, that's hard to do with 900 commits accumulated. The only way to make it simple is to do it like that from the start. (Which puts a bit of additional effort on multiple contributors to the same branch, but is a win even in the middle-run.)

Member

pfalcon commented Jul 18, 2018

I would have preferred even more history being kept

I'm all for keeping more history. But the history should be clean. It should help answer questions, not confuse someone seeking for answers. I gave an example with 7c74b7d - the commit message doesn't explain why the change is made, so for someone exploring history of ports/nrf/drivers/softpwm.c file, that commit will be just noise. It should have been squashed into the commit which named that function softpwm_init(), so it came out with the right name, softpwm_init0(), in the history right away.

Of course, that's a minor case, but hundreds of such minor cases make history exploration incomprehensible. And work on any development branch (and any port, until it's merged, is a development branch) involves squashing and rebasing to maintain history clean. That's how @dpgeorge works on the branches, and how I worked on them too (and keep working in my fork). Of course, that's hard to do with 900 commits accumulated. The only way to make it simple is to do it like that from the start. (Which puts a bit of additional effort on multiple contributors to the same branch, but is a win even in the middle-run.)

@aykevl

This comment has been minimized.

Show comment
Hide comment
@aykevl

aykevl Jul 19, 2018

Contributor

I don't know about softpwm_init0 but I'm guessing it's name was changed to keep it consistent with the other initializers (see ports/nrf/main.c).

I think we mostly agree, and I've tried to do it as much as possible. For example, we had a branch to switch to a new HAL that contained tens of commits that were all squashed together as this switch really is one (very big) change. Unfortunately, changing history is very inconvenient with multiple contributors, especially if you're working on multiple branches in that fork at the same time (which we did). It is a much smaller problem with regular feature branches.

Contributor

aykevl commented Jul 19, 2018

I don't know about softpwm_init0 but I'm guessing it's name was changed to keep it consistent with the other initializers (see ports/nrf/main.c).

I think we mostly agree, and I've tried to do it as much as possible. For example, we had a branch to switch to a new HAL that contained tens of commits that were all squashed together as this switch really is one (very big) change. Unfortunately, changing history is very inconvenient with multiple contributors, especially if you're working on multiple branches in that fork at the same time (which we did). It is a much smaller problem with regular feature branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment