nRF52832 and nRF52 DK Support

nocko commented 4 years ago

This is working (examples included), but rather less complete than nRF51.

PWM / Analog Out is untested... but even if it works, it should be updated to use the PWM peripherals of the nRF52
The ADC is untested. This peripheral has changed entirely. I scraped together something into sort of the old API... but probably this API should change entirely
Only ZFP support
~~Controversial (?) removal of System.Interrupts from nvic_cm4_cm7 in favour of locally defined types (like cm0)... due to only supporting ZFP.~~
~~Many peripherals are the same between nrf51 and 52, but this implementation copies all peripheral drivers. There may be a way to combine them intelligently instead.~~

Feedback appreciated.

Fabien-Chouteau commented 4 years ago

Thank you for your contribution @nocko :)

I see the nRF52 in many boards these days so it's great to have support for it.

Just by quickly looking at your pull-request, it looks like there is a lot of code duplication from the nRF51 drivers. If that's the case it would be better to share the code when possible, I can help you find a solution for that.

You also copied all the micro:bit examples but they do not apply to the nRF52_DK.

For the ravenscar-sfp support, what do you have in mind? Because if the drivers work on ZFP they will also work on the ravenscar-sfp.

For system_nrf52.c, the code doesn't look very big but on the other hand drags a lot of headers. So it better to rewrite it in Ada and, for instance, put it in the elaboration code of the nRF52 package.

Did you use startup-gen to generate the crt0 and linker script?

nocko commented 4 years ago

Hi Fabien!

De-duplication

I thought it over last night and de-duplication is still the [obviously] better case though (esp. because I am planning nRF52840 and 810 support as well). I am thinking that I'll split into family/chip specific source directories, but keep everything under a common package (NRF? Nordic?). That should avoid programmers having to manually remember a package path for each peripheral, and limit changes to managing the source directories RTS gpr file. This wouldn't work (cleanly) if two chips had different peripherals (required different drivers) but the same name. I don't think this is currently the case...

Examples

I only ported three (digital_out, BLE_beacon, and buttons) examples from Microbit and all three are working on the nRF52 DK. I think this should be okay.

startup-gen

I didn't use startup-gen. I manually edited the linker script and ported crt0 from the microbit (and included the vendor files for the [huge] list of errata. I'll investigate, but see question 1 below.

Questions

These Nordic chips have huge errata. Currently the nRF51 support doesn't seem to enable any of the recommended work-arounds. Can we keep the vendor files for errata abatement or should I rewrite in Ada? Regardless, of which is there a way to specify extra startup code without manually editing crt0.S generated by startup-gen (or running a "Fix_Errata" procedure first in every main unit).
Does my plan for de-duplication above seem reasonable? I'd certainly accept any other hints on how to do this best.

Fabien-Chouteau commented 4 years ago

De-duplication

I thought it over last night and de-duplication is still the [obviously] better case though (esp. because I am planning nRF52840 and 810 support as well). I am thinking that I'll split into family/chip specific source directories, but

keep everything under a common package (NRF? Nordic?).

Yes, that's what we do for STM32 packages, and I should have done the same with nRF51. Please use nRF. as this is the common part of the device family name.

This wouldn't work (cleanly) if two chips had different peripherals (required different drivers) but the same name. I don't think this is currently the case...

Actually you can then differentiate the drivers by putting them in different directories:

/drivers/SPI_nRF51/ vs /drivers/SPI_nRF52

Examples

I only ported three (digital_out, BLE_beacon, and buttons) examples from Microbit and all three are working on the nRF52 DK. I think this should be okay.

If they work yes, that's ok. But in that case please update the readmes because they are mentioning micro:bit specific stuff.

startup-gen

I didn't use startup-gen. I manually edited the linker script and ported crt0 from the microbit (and included the vendor files for the [huge] list of errata. I'll investigate, but see question 1 below.

You should use startup-gen, there is already everything in place to use it in Ada Drivers Library (see how it is done for the micro:bit).

Questions

1. These Nordic chips have _huge_ errata. Currently the nRF51 support doesn't seem to enable any of the recommended work-arounds. Can we keep the vendor files for errata abatement or should I rewrite in Ada? Regardless, of which is there a way to specify extra startup code without manually editing crt0.S generated by startup-gen (or running a "Fix_Errata" procedure first in every main unit).

I do think that we should implement the errata in Ada. The good way to integrate them without editing the crt0.S is to use elaboration code. For instance:

package body nRF is

begin
   if Errata_X then
      --  Fix errata
   end if;
end nRF;

This code will be automatically executed when Ada is initialized.

2. Does my plan for de-duplication above seem reasonable? I'd certainly accept any other hints on how to do this best.

Yes they do, I seems similar to what we do for STM32. You should have a look at that.

Don't hesitate to ask me for feedback as soon as you can. Here or in the gitter char.

Thanks,

nocko commented 4 years ago

@Fabien-Chouteau CI is caught up in svd2ada generated files.

Example:

   compilation of nrf_svd-swi.ads failed

Build error (gprbuild returned 4):
nrf_svd-swi.ads:37:06: warning: unit "System" is not referenced
gprbuild: *** compilation phase failed

Others are records whose contents don't fill the entire Size:

nrf_svd-mwu.ads:2721:19: warning: 64 bits of "REGION_Cluster" unused

Should I hand edit these for pedantics, or file bugs against svd2ada? I doubt there are actual bugs in svd2ada... it's just a question if it's in scope for svd2ada to try to completely work around the badness in vendor files.

As an aside, almost all the driver changes between nrf51 and nrf52 are working around the differences in naming in the svd files, rather than any substantial changes.

nocko commented 4 years ago

Status

[x] Common packages (nRF & NRF_SVD)
[x] Passing CI
[x] Driver sharing maximized via re-generating nrf51.svd and separate subroutines
[x] Conversion to startup-gen
[x] nRF52 and nRF52_DK added project_wizard and boards_project_generator
[x] Update documentation for examples
[x] Errata abatement in package elaboration

nocko commented 4 years ago

I think this is ready for review. There's an opertunity to combine some of the microbit board-level drivers with the nrf52 board drivers, but this patch is already so big that I think that should be addressed in a follow up after this work lands. @Fabien-Chouteau

nocko commented 4 years ago

Rebased on master after the cm0 NVIC function renaming.

Fabien-Chouteau commented 4 years ago

Thanks for the update @nocko. I will send a PR to your branch for a few type changes on the NVIC drivers.

For nrf_svd-swi.ads, I think you can remove it. It is empty anyway.

nocko commented 4 years ago

@Fabien-Chouteau Shall I squash these commits?

Fabien-Chouteau commented 4 years ago

Shall I squash these commits?

Don't worry I will probably do a squash and merge on the PR anyway.

Fabien-Chouteau commented 4 years ago

Hi @nocko, excuse me for the delay I was not available the last couple of weeks. I will have a look at the PR as soon as possible.

nocko commented 4 years ago

Thanks @Fabien-Chouteau! I understand that life often "gets in the way". Whenever you are ready is fine with me.

Fabien-Chouteau commented 4 years ago

Thank you @nocko ! It's great to have nRF52 support in Ada Drivers Library :)

AdaCore / Ada_Drivers_Library