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

Tracking: Refactor nrf52 boards organization #1839

Closed
7 of 10 tasks
hudson-ayers opened this issue May 8, 2020 · 10 comments · Fixed by #1892
Closed
7 of 10 tasks

Tracking: Refactor nrf52 boards organization #1839

hudson-ayers opened this issue May 8, 2020 · 10 comments · Fixed by #1892
Assignees
Labels

Comments

@hudson-ayers
Copy link
Contributor

hudson-ayers commented May 8, 2020

The discussion in #1784 seems to have arrived at the conclusion that the organization of the nordic boards no longer makes sense, now that we have components and support multiple non-development-kit nordic boards.

The desired result is to refactor the nordic boards such that each board has its own folder (like how all other boards are handled), and use components for all initialization to minimize the repeated code between board files.

Below are a series of largely independent steps that would get us to that result:


Once those steps are done..

  • Create individual directories for nrf52dk, nrf52840dk, and nrf52840_dongle in the top level of boards/, each with its own main.rs that copies in the completely-componentized setup_board() currently shared by those three boards. Delete boards/nordic/ (Run OpenTitan tests in the CI #1859)
@bradjc
Copy link
Contributor

bradjc commented May 8, 2020

  1. Why delete boards/nordic? It seems like grouping all of the nordic produced boards has served us well so far.

  2. Why would nrf52dk_base still be necessary?

@hudson-ayers
Copy link
Contributor Author

  1. Why delete boards/nordic? It seems like grouping all of the nordic produced boards has served us well so far.

Well, I always thought it was a little weird that the acd52832 wasn't in that folder (I realize that the board isn't made by nordic, just the chip, but still..), and also that we only do it for nordic boards but not, say, stm boards. But we could certainly still keep the folder.

  1. Why would nrf52dk_base still be necessary?

It wouldn't be once all the steps are done. I was trying to express that initially we should componentize everything in nrf52dk_base/src/lib.rs::setup_board(), and once that is done we should delete nrf52dk_base

@gendx
Copy link
Contributor

gendx commented May 11, 2020

  1. Why delete boards/nordic? It seems like grouping all of the nordic produced boards has served us well so far.

I always found it weird that Nordic boards were the only ones grouped together. I think we should either have a flat folder containing all the boards, or have a folder for each manufacturer (even if most manufacturers have only one board in them). Otherwise a mix of "categorized" boards (within nordic/) and "uncategorized" boards directly at the top level seems weird.

@hudson-ayers hudson-ayers self-assigned this May 14, 2020
bors bot added a commit that referenced this issue May 20, 2020
1853: Components for all things nrf52 r=bradjc a=hudson-ayers

### Pull Request Overview

This pull request creates components for all portions of nrf52dk-specific initialization that does not already rely on components.

Specifically:

- Converts Imix-specific analog comparator component into generic analog comparator component, uses it for Imix and nrf52*

- Converts Imix-specific nonvolatile storage component into generic component, uses it for Imix and nrf52dk (required some HIL changes!)

- Converts si7021 specific temperature sensor component into generic temperature sensor component, uses it for Imix, nrf52*, and Hail.

- Creates a component for standalone initialization of the mx25r6435f flash chip used on the nrf52840dk

- Creates NrfStartupComponent and NrfClockComponent, to store repeated initialization code in functions that can be reused.

- Uses the existing SPI compnents to initialize SPI for the nrf52* boards.


This PR is the first step towards addressing #1839 .

### Testing Strategy

This pull request was tested by running the in-kernel log_test on Imix, running the nonvolatile_storage test on Imix, running the analog_comparator test on Imix, and running the analog_comparator and nonvolatile_storage tests on the nrf52840dk.


### TODO or Help Wanted

I am not sure if adding a `configure()` function to the flash HIL was the correct call given that only the sam4l flash controller requires it.

### Documentation Updated

- [x] No updates are required?

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Hudson Ayers <hayers@stanford.edu>
@bradjc
Copy link
Contributor

bradjc commented May 29, 2020

I don't think we should have a policy/preference around this. Using folders seems like it could be good in some cases. Flat list is also useful.

But moving code around often leads to annoying git blames, so I prefer to avoid it unless it is really necessary.

@ppannuto
Copy link
Member

I do think we should have some policy around this. The blame issue is less bad if you've aliased it to always set --follow, but that is imperfect (and doesn't fix blame on github web).

I think it's confusing as it's sort of ad-hoc. Are they grouped by nordic because they use nordic chips (then why isn't acd52832 in that folder)? Are they grouped by nordic because nordic makes those boards (then why do we have three stm folders)?

I think we should decide on some policy and pay the rename cost once en masse.

@alistair23
Copy link
Contributor

For longer term grouping I think it makes sense to try and group boards by either:

  1. The chip maker
    • boards/sifive would contain all boards with a SiFive SoC.
    • boards/nordic would contain all boards with a Nordic SoC.
  2. The board maker
    • boards/sifive would contain all boards made by SiFive
    • boards/sparkfun would contain all the boards by SparkFun
  3. Flat (no grouping)

Both 1 and 2 work better with some boards then they do with others. 3 could have redundancies especially for vendor tools like flash scripts.

@bradjc
Copy link
Contributor

bradjc commented May 29, 2020

I think there can be a lot of ways to group things, and different approaches might make sense in different cases. For the nordic created boards, the maintainers of those boards thought grouping them made sense, so we grouped them. Grouping the STM boards might also make sense. We might want to group by project (say "signpost" for example). Or by board family. Or board type if there are multiple revisions (say "telos" for example). Who knows.

@alistair23
Copy link
Contributor

The problem is that it can become confusing for new users to find what boards Tock supports

@gendx
Copy link
Contributor

gendx commented Jun 2, 2020

I'm more worried about new users being confused in navigating the code than by git blame being less friendly. Browsing the source directories is a much more common use case than doing a git blame, and for the latter use case @ppannuto mentioned the --follow flag.

I think it's important to make things easy for newcomers ; people who are already more familiar with the code can afford to add a --follow flag when they perform more advanced actions like git blame.

Likewise, I don't think that the maintainers of those boards thought grouping them made sense, so we grouped them is a good policy. It encourages fragmentation within Tock, making it harder to do things like porting common logic across boards.
Similar pitfalls would be:

  • maintainers of board A thought having components made sense, so A uses components, but maintainers of board B were opposed to the overhead of components, so B doesn't use a single component
  • maintainers of board A thought using Cargo features made sense, so A uses features, but maintainers of board B were opposed to the complexity of features, so B doesn't use a single feature.

@bradjc
Copy link
Contributor

bradjc commented Jun 2, 2020

I'm not worried about a slippery slope here. We clearly want to use components, and we don't like features. There isn't much ambiguity there.

My opinion on the way to make this more friendly is tooling and documentation. A strong README is much more important than ls /boards listing a long list of somewhat cryptic board names. We also have make list.

--follow doesn't work online, which makes it not a useful option for me.

What about the other reasons folders might be useful? If we had, say, 100 supported boards, would we really not want to be able to group them?

@bors bors bot closed this as completed in 01f453a Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants