Skip to content

boards/atmega328p-based: Model features in Kconfig#14176

Merged
fjmolinas merged 8 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/kconfig/atmega_boards_symbols
Jun 17, 2020
Merged

boards/atmega328p-based: Model features in Kconfig#14176
fjmolinas merged 8 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/kconfig/atmega_boards_symbols

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri commented May 29, 2020 •
edited
Loading

Contribution description

Similarly to #13404, this models the symbols for all boards based on the atmega328p CPU, and selects the features provided.

Testing procedure

  • Review the symbol organization and the values assigned to the common symbols
  • The test provided in boards/samr21-xpro: Model features in Kconfig #13404 should pass
  • tests/kconfig_features should pass for all the atmega328p-based boards:
    • arduino-duemilanove
    • arduino-nano
    • arduino-uno
    • atmega328p

Issues/PRs references

Depends on #13404

@leandrolanzieri leandrolanzieri added Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first Area: Kconfig Area: Kconfig integration labels May 29, 2020
@jia200x jia200x mentioned this pull request May 29, 2020
Closed
@leandrolanzieri leandrolanzieri added the Platform: AVR Platform: This PR/issue effects AVR-based platforms label Jun 2, 2020
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig/atmega_boards_symbols branch from 7818354 to e45cfea Compare June 3, 2020 08:16
@leandrolanzieri leandrolanzieri removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 3, 2020
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Jun 3, 2020

Rebased and added the boards to the whitelist of the test

@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 3, 2020
@leandrolanzieri leandrolanzieri mentioned this pull request Jun 3, 2020
Merged
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig/atmega_boards_symbols branch 2 times, most recently from a0fdd68 to 8219870 Compare June 3, 2020 13:11
@leandrolanzieri leandrolanzieri mentioned this pull request Jun 4, 2020
Merged
@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 4, 2020
This was referenced Jun 4, 2020
Merged
Merged
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig/atmega_boards_symbols branch from 8219870 to 0837322 Compare June 5, 2020 10:58
@leandrolanzieri leandrolanzieri mentioned this pull request Jun 5, 2020
Merged
@fjmolinas fjmolinas self-assigned this Jun 9, 2020
fjmolinas
fjmolinas reviewed Jun 9, 2020
View reviewed changes
fjmolinas
fjmolinas reviewed Jun 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I general everything looks good regarding the 1:1 mapping to FEATURES_PROVIDED, but I have some questions regarding the implementation choices.

@leandrolanzieri leandrolanzieri mentioned this pull request Jun 10, 2020
Merged
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 16, 2020

Discussed offline with @leandrolanzieri about the current CPU_% symbols. The conclusion is that CPU_LINE is not very clear, in many cases it would result in defining many CPU_COMMON_% in addition to CPU_LINE and the distinction between them would not be very clear. Therefore we agreed with keeping the specific symbols that have a clear definition, so the following graph:

   +-----------+
   | CPU_MODEL |
   +-----------+
         |
         v
   +-----------+
   |  CPU_FAM | 
   +-----------+         
         |               
         v               
   +-----------+
   |  CPU_CORE | 
   +-----------+ 
         |
         v
   +-----------+
   |  CPU_ARCH |
   +-----------+

CPU_COMMON_% will be used for cases where additional grouping is useful in terms of code maintenance or to reduce code duplication. If later on we can pinpoint a clear definition that would require a new explicit category (CPU_LINE or another) we can extend the definitions accordingly. But for now having the distinction is adding noise to the migration process without contributing much useful information about these categories.

So what we want to do is:

  • change existing models to adjust to the above graph
  • open a PR documenting this
  • rebase this PR and the others that are already opened

Did I miss something @leandrolanzieri ?

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Jun 16, 2020

Did I miss something @leandrolanzieri ?

Nope, that's a good summary.

@leandrolanzieri leandrolanzieri mentioned this pull request Jun 16, 2020
Merged
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 17, 2020

@leandrolanzieri this one needs rebasing!

@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig/atmega_boards_symbols branch from 0837322 to 71eb252 Compare June 17, 2020 08:53
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig/atmega_boards_symbols branch from 71eb252 to 4a1f6e5 Compare June 17, 2020 08:59
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Jun 17, 2020

@fjmolinas rebased and adapted to new classification

fjmolinas
fjmolinas approved these changes Jun 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, my changes have been addressed, lets see what murdock has to say :)

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Jun 17, 2020

All green @fjmolinas :)

@leandrolanzieri leandrolanzieri added this to the Release 2020.07 milestone Jun 17, 2020
@fjmolinas fjmolinas merged commit 112ff81 into RIOT-OS:master Jun 17, 2020
@leandrolanzieri leandrolanzieri deleted the pr/kconfig/atmega_boards_symbols branch June 17, 2020 10:25
@leandrolanzieri leandrolanzieri mentioned this pull request Jun 29, 2020
Merged
@leandrolanzieri leandrolanzieri mentioned this pull request Sep 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@benpicco benpicco Awaiting requested review from benpicco

@maribu maribu Awaiting requested review from maribu

@cgundogan cgundogan Awaiting requested review from cgundogan

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

Assignees

@fjmolinas fjmolinas

Labels

Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2020.07

Development

Successfully merging this pull request may close these issues.

3 participants

@leandrolanzieri @fjmolinas @aabadie