Refactor samd21 #5743

keestux · 2016-08-10T20:11:38Z

This is the fourth attempt to reorganize cpu/samd21 so that it will be possible to add new boards like SODAQ Autonomo and Arduino Zero.

There are 11 commits which do the following steps

move the current samd21 include files to sam21_common (only CMSIS)
update the samr21 CMSIS from ASF 3.21
add the samd21 CMSIS from ASF 3.30
move i2c to sam21_common
refactor samd21 spi and uart

Notice that there is no new "cpu" and a start is made to move common code to sam21_common. The only thing I want to mention is that samd21 boards now need a few extras in their Makefile.include, like

export CPU = samd21
export SAM0_FAM = samd21
export CPU_MODEL = samd21j18a   # Not sure if this is used
CFLAGS += -D__SAMD21J18A__  # or whatever variant you have

This is already taken care of in samr21-xpro.

@PeterKietzmann was kind enough to already test these changes on a samr21-xpro and confirmed that everything continues to work.

PeterKietzmann · 2016-08-11T07:13:39Z

Yes I did test UART, I2C and SPI with this branch on a samr21-xpro and everything worked as expected. Also note that Murdocks output looks pretty nice. It just complains about whitespace errors in CMSIS headers. How do we handle that?

kaspar030 · 2016-08-11T07:32:27Z

Murdocks output looks pretty nice. It just complains about whitespace errors in CMSIS headers. How do we handle that?

Add the dirs to the EXCLUDE_PATTERNS in doc/doxygen/riot.doxyfile.

kaspar030 · 2016-08-11T07:35:11Z

cpu/sam21_common/Makefile.include

+CFLAGS += -DCPU_FAM_$(shell echo $(CPU_FAM) | tr 'a-z-' 'A-Z_')
+
+# Define the SAM0 family so we can differentiate between them in the code
+CFLAGS += -DSAM0_FAM_$(shell echo $(SAM0_FAM) | tr 'a-z-' 'A-Z_')


Any reason to not use CPU_FAM instead of SAM0_FAM? Seems redundant.

I looked at it, but the problem is that CPU_FAM is currently set in cpu/samd21/Makefile.include. And that one is shared for samd21 and samr21 boards. We need a parameter to distinguish between these two.

Notice that for samr21-xpro SAM0_FAM is set to samr21.

That why I came up with SAM0_FAM which must be set in the board configuration.

I looked at it, but the problem is that `CPU_FAM` is currently set in `cpu/samd21/Makefile.include`. And that one is shared for samd21 and samr21 boards. We need a parameter to distinguish between these two.

Why? We don't have to use the dispatching from CMSIS. We currently support one samd21 CPU (SAMR21G18A), and that number might rise to 5-10 in the mid-term. IMHO we should just select the right cmsis header directly (in cpu_conf.h). Reduces "-DCPU_FAM=samd21 -DSAM0_FAM=samr21 -D__SAMR21G18A__ -DCPU_FAM_SAMD21 -DSAM0_FAM_SAMR21" by 40%. ;)

Why do you pick on me? I'd say, look at your own contribution of cpu/saml21. It has the whole set of cmsis for saml21.

IMHO we should just select the right cmsis header directly (in cpu_conf.h).

You can't. Not without adding a define like, eh, SAM0_FAM_xyz. If you had let me move samd21 to samr21 and add the 'real' samd21, then the three samd21, samr21, saml21 would all have had the same structure. And SAM0_FAM wouldn't be needed.

jnohlgard · 2016-08-11T07:36:22Z

@keestux Thank you for your patience with this tedious task.

Please fix the whitespace errors (trailing whitespaces) in the CMSIS headers as a separate commit.
find cpu/samr21/include -name '*.h' -exec sed -i 's/\s*$//' + should fix it I think.

edit: added missing -i in the sed command line

kaspar030 · 2016-08-11T07:37:13Z

Are the samd21 CMSIS headers used anywhere with this PR? If not please cut them out of this PR for better reviewability, and add them together with the first board using them (autonomo?)

kaspar030 · 2016-08-11T07:37:50Z

@keestux Thank you for your patience with this tedious task.

+1!

keestux · 2016-08-11T19:13:06Z

@kaspar030 About the samd21 CMSIS files. I rather keep them in.
I needed this sequence of commits to see if samr21-xpro and sodaq-autonomo could coexist.

kaspar030 · 2016-08-13T07:32:26Z

Why do you pick on me? I'd say, look at your own contribution of cpu/saml21. It has the whole set of cmsis for saml21.

I'm sorry if you have the impression that I'm picking on you. That's definitely not my intend. Feel invited for a drink the moment we meet in person! :)

IMHO we should just select the right cmsis header directly (in cpu_conf.h).
You can't. Not without adding a define like, eh, SAM0_FAM_xyz. If you had let me move samd21 to samr21 and add the 'real' samd21, then the three samd21, samr21, saml21 would all have had the same structure. And SAM0_FAM wouldn't be needed.

Well, I think we shouldn't let the vendor headers influence too much on how we best organize our platforms.

The samr21 is just a variant of samd21, even if Atmel is shipping a whole new include tree. A diff shows that they are practically identical.

Please take a look at keestux#1.

In there I've just copied the four samr21 specific files into the samd21 cmsis folder, and added the extra case to samd21.h. Works fine as is, saves 20k lines of duplicated header files and removes the need for SAM0_FAM.

jnohlgard · 2016-08-13T08:12:29Z

Den 13 aug. 2016 09:32 skrev "Kaspar Schleiser" notifications@github.com:

Why do you pick on me? I'd say, look at your own contribution of
cpu/saml21. It has the whole set of cmsis for saml21.

I'm sorry if you have the impression that I'm picking on you. That's
definitely not my intend. Feel invited for a drink the moment we meet in
person! :)

IMHO we should just select the right cmsis header directly (in
cpu_conf.h).
You can't. Not without adding a define like, eh, SAM0_FAM_xyz. If you
had let me move samd21 to samr21 and add the 'real' samd21, then the three
samd21, samr21, saml21 would all have had the same structure. And SAM0_FAM
wouldn't be needed.

Well, I think we shouldn't let the vendor headers influence too much on
how we best organize our platforms.

The samr21 is just a variant of samd21, even if Atmel is shipping a whole
new include tree. A diff shows that they are practically identical.

Please take a look at keestux#1.

In there I've just copied the four samr21 specific files into the samd21
cmsis folder, and added the extra case to samd21.h. Works fine as is, saves
20k lines of duplicated header files and removes the need for SAM0_FAM.

Keep in mind the maintainability of the files. I don't see any problem with
vendor headers duplicating their content when it reduces our own
maintenence effort, so I don't agree with your opinion, Kaspar. But I just
want this issue to be resolved so that we can focus on more productive
tasks.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub, or mute the thread.

kaspar030 · 2016-08-13T09:20:29Z

well, I promise to script the merging of the two cmsis trees the next time one of them needs an update.

(edit removed email leftovers)

jnohlgard · 2016-08-13T09:35:06Z

Okay, agreed. Let's get this in so that @keestux can get back to working on
the port for the new boards.
@kaspar030 Will you do the review ?

Den 13 aug. 2016 11:20 AM skrev "Kaspar Schleiser" <notifications@github.com

:

well, I promise to script the merging of the two cmsis trees the next time
one of them needs an update.

-----Original Message-----
From: "Joakim Nohlgård" notifications@github.com
To: RIOT-OS/RIOT RIOT@noreply.github.com
Cc: Kaspar Schleiser kaspar@schleiser.de, Mention <
mention@noreply.github.com>
Sent: Sa., 13 Aug. 2016 10:12
Subject: Re: [RIOT-OS/RIOT] Refactor samd21 (#5743)

Den 13 aug. 2016 09:32 skrev "Kaspar Schleiser" <notifications@github.com

:

Why do you pick on me? I'd say, look at your own contribution of
cpu/saml21. It has the whole set of cmsis for saml21.

I'm sorry if you have the impression that I'm picking on you. That's
definitely not my intend. Feel invited for a drink the moment we meet in
person! :)

IMHO we should just select the right cmsis header directly (in
cpu_conf.h).
You can't. Not without adding a define like, eh, SAM0_FAM_xyz. If you
had let me move samd21 to samr21 and add the 'real' samd21, then the three
samd21, samr21, saml21 would all have had the same structure. And SAM0_FAM
wouldn't be needed.

Well, I think we shouldn't let the vendor headers influence too much on
how we best organize our platforms.

The samr21 is just a variant of samd21, even if Atmel is shipping a whole
new include tree. A diff shows that they are practically identical.

Please take a look at keestux#1.

In there I've just copied the four samr21 specific files into the samd21
cmsis folder, and added the extra case to samd21.h. Works fine as is, saves
20k lines of duplicated header files and removes the need for SAM0_FAM.

Keep in mind the maintainability of the files. I don't see any problem with
vendor headers duplicating their content when it reduces our own
maintenence effort, so I don't agree with your opinion, Kaspar. But I just
want this issue to be resolved so that we can focus on more productive
tasks.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub, or mute the thread.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5743 (comment)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#5743 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATYQo4P4UpuPqOpbhdXd2KGW0LlPiI9ks5qfYxegaJpZM4Jhh2G
.

kaspar030 · 2016-08-13T09:48:36Z

well, picking on keestux was me already reviewing. :)

as the samr21 is probably our most used board, I'll do more thorough testing. won't get to it until tomorrow, though.

-----Original Message-----
From: "Joakim Nohlgård" notifications@github.com
To: RIOT-OS/RIOT RIOT@noreply.github.com
Cc: Kaspar Schleiser kaspar@schleiser.de, Mention mention@noreply.github.com
Sent: Sa., 13 Aug. 2016 11:35
Subject: Re: [RIOT-OS/RIOT] Refactor samd21 (#5743)

Okay, agreed. Let's get this in so that @keestux can get back to working on
the port for the new boards.
@kaspar030 Will you do the review ?

Den 13 aug. 2016 11:20 AM skrev "Kaspar Schleiser" <notifications@github.com

:

well, I promise to script the merging of the two cmsis trees the next time
one of them needs an update.

-----Original Message-----
From: "Joakim Nohlgård" notifications@github.com
To: RIOT-OS/RIOT RIOT@noreply.github.com
Cc: Kaspar Schleiser kaspar@schleiser.de, Mention <
mention@noreply.github.com>
Sent: Sa., 13 Aug. 2016 10:12
Subject: Re: [RIOT-OS/RIOT] Refactor samd21 (#5743)

Den 13 aug. 2016 09:32 skrev "Kaspar Schleiser" <notifications@github.com

:

Why do you pick on me? I'd say, look at your own contribution of
cpu/saml21. It has the whole set of cmsis for saml21.

I'm sorry if you have the impression that I'm picking on you. That's
definitely not my intend. Feel invited for a drink the moment we meet in
person! :)

IMHO we should just select the right cmsis header directly (in
cpu_conf.h).
You can't. Not without adding a define like, eh, SAM0_FAM_xyz. If you
had let me move samd21 to samr21 and add the 'real' samd21, then the three
samd21, samr21, saml21 would all have had the same structure. And SAM0_FAM
wouldn't be needed.

Well, I think we shouldn't let the vendor headers influence too much on
how we best organize our platforms.

The samr21 is just a variant of samd21, even if Atmel is shipping a whole
new include tree. A diff shows that they are practically identical.

Please take a look at keestux#1.

In there I've just copied the four samr21 specific files into the samd21
cmsis folder, and added the extra case to samd21.h. Works fine as is, saves
20k lines of duplicated header files and removes the need for SAM0_FAM.

Keep in mind the maintainability of the files. I don't see any problem with
vendor headers duplicating their content when it reduces our own
maintenence effort, so I don't agree with your opinion, Kaspar. But I just
want this issue to be resolved so that we can focus on more productive
tasks.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub, or mute the thread.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5743 (comment)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#5743 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATYQo4P4UpuPqOpbhdXd2KGW0LlPiI9ks5qfYxegaJpZM4Jhh2G
.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5743 (comment)

PeterKietzmann · 2016-08-15T06:12:43Z

@kaspar030 I can give a bit support in detailed testing. Already did a quick test with networking, spi and i2c which worked well.

kaspar030 · 2016-08-15T10:55:13Z

Ok, you convinced me that fiddling with the cmsis files or their structure is a bad idea compared to the small overhead of duplicated/identical vendor headers.

Still, @keestux, I would like to get rid of the SAM0_FAM C defines. As I see it, it is only used for selecting the correct cmsis include path (in the makefiles) and include file.

Could you live with creating a sam0.h which basically looks the same as e.g., samr21.h, but listing all sam0 variants we support, that either includes the correct saml21a12b.h header or just passes on to saml21.h and samr21.h?

keestux · 2016-08-15T19:27:09Z

@kaspar030 About the sam0.h, yes that seems possible. I'll give it a try.

keestux · 2016-08-15T21:08:37Z

@kaspar030 I've update the branch with the suggested sam0.h

kYc0o · 2016-08-19T10:03:43Z

As in #5525 Murdock doesn't seem to finish here too...

In the Makefile.include of the board it is required to set the correct define. Currently only SAMRG18A is supported, as required by samr21-xpro.

A generic function is added to initialize a SERCOM. Notice that uart_conf_t was expanded with pad settings, but it isn't used yet.

Notice that saml21 does not use I2C (yet).

The former i2c_init_master was too complicated. It was trying to set IN/OUT mode of the pins, but all that is needed is to set the proper MUX. Also the configuration for the boards was incomplete (no MUX, no pad setting). It was tested on a SODAQ Autonomo, but not on a samr21-xpro

The board config can define SPI_1_EN as 0, and in that case #ifdef won't work. Add some more comments, and fix typos.

The pinmux is now part of the board config. The pad setting is done with clear names instead of numbers.

Notice the extra fields in uart_conf_t for rx_pad and tx_pad.

…ommon

This define conflicts with LITTLE_ENDIAN defined in include/machine/endian.h which is part of gcc-arm-none-eabi. Also, the define does not seem to be used by the ASF included files.

keestux · 2016-09-21T19:06:39Z

@kaspar030 Update is available

rebase, done
dropped the addition of samd21 (they'll be added with upcoming PRs)
squashed, not done (I wanted to keep each commit as is, because each commit tells its own story. It will be easier to "blame" if something turned out to introduce bugs, which of course is very unlikely :-) )

kaspar030 · 2016-09-22T12:54:44Z

ACK.

immesys · 2016-09-22T22:53:31Z

Wow, great work @keestux. I'm also putting together a board based on SAMR21, so I appreciate you cleaning stuff up!

haukepetersen · 2016-10-05T11:03:01Z

I know this PR took a long road and I support the effort for a cleaner structure in the Atmel CPU tree of RIOT. But I think here are some/many things that went wrong with this PR! My main issues are:

why host the samr/d21 header in the sam21_common, while the saml21 header are not? -> needs unification
most of the commits do more/different things then they say in their commit message
bad code style was merged (e.g. c++-style comments in cpu/samd21/periph/gpio.c
functionality unrelated to this PR was merged -> adding/changing some sercom functionality for periph drivers is completely unrelated to this PR
functions where not reviewed properly: the newly added gpio_init_sercom() does exactly the same than the already existing gpio_init_mux() function
etc...

@kYc0o @PeterKietzmann @kaspar030 I know this PR took a long road, but let's not relax our requirements on code quality and PR structure!

@keestux I really appreciate your effort! Maybe next time try to go in small steps, which makes things so much easier (and hopefully faster) to review and merge.

keestux · 2016-10-06T19:59:46Z

@haukepetersen Hmm. You did notice that this already got merged, right?

About saml21 in sam21_common, but no headers. Preferably saml21 should go in there as well.
I really try to make a commit do one thing only.
C++ style comment are not documented as "bad code style". The wiki states: "code shall be C99 compliant", which allows //. This code in particular was copied from Arduino. And in these cases I want to minimize the differences with the original.
Unrelated functionality was added. Well, without these changes it was not easy to deliver working commits. I did my best to first send in changes with the ultimate goal to have my board supported at some point. It was a battle.
functions were not reviewed properly? Hmm.
etc? Keep going. I can handle it :-)

Small steps? Like #4780 ? ;-)

miri64 · 2016-10-07T13:52:27Z

Small steps? Like #4780 ? ;-)

I think what Hauke is trying to say is that though there is no convention as to how big a PR is to be, this is - as a rule of thumb 😜 - generally not good for review:

immesys · 2016-10-07T16:01:12Z

As an outside, I must agree. Even after I knew that this PR was responsible for #5868, searching through the changes made in the PR took a very long time.

edit: although I really really appreciate that it was not squashed and the commits within the PR were distinct

OlegHahm · 2016-10-12T17:30:50Z

C++ style comment are not documented as "bad code style". The wiki states: "code shall be C99 compliant", which allows //.

Our coding conventions are inherit from the Linux coding conventions. See section 8 in
https://www.kernel.org/doc/Documentation/CodingStyle.

PeterKietzmann added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 11, 2016

PeterKietzmann added this to the Release 2016.10 milestone Aug 11, 2016

PeterKietzmann assigned kaspar030, kYc0o and PeterKietzmann Aug 11, 2016

PeterKietzmann 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 Aug 11, 2016

kaspar030 reviewed Aug 11, 2016
View reviewed changes

PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 12, 2016

kaspar030 mentioned this pull request Aug 15, 2016

samd21: merge samr21 variant into samd21 cmsis headers keestux/RIOT#1

Closed

keestux force-pushed the refactor-samd21 branch from d1a622e to b0ed884 Compare August 15, 2016 21:07

kaspar030 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 Aug 19, 2016

Kees Bakker added 13 commits September 21, 2016 19:43

cpu/samd21: introduce sam0.h so that we can select samr21.h or other

8644b5f

In the Makefile.include of the board it is required to set the correct define. Currently only SAMRG18A is supported, as required by samr21-xpro.

cpu/sam21_common: add a few typedefs for SERCOM

4ad3313

A generic function is added to initialize a SERCOM. Notice that uart_conf_t was expanded with pad settings, but it isn't used yet.

cpu/sam21_common: move i2c.c from samd21 to sam21_common

be1113f

Notice that saml21 does not use I2C (yet).

cpu/samd21: add helper functions spi_power_on/off

574b565

cpu/samd21: spi: change #ifdef's to #if's

f794bd4

The board config can define SPI_1_EN as 0, and in that case #ifdef won't work. Add some more comments, and fix typos.

cpu/samd21: Refactor spi using ArduinoCore as example

845ef0d

The pinmux is now part of the board config. The pad setting is done with clear names instead of numbers.

cpu/samd21: uart: now using padd setting from board config

ea8b90f

Notice the extra fields in uart_conf_t for rx_pad and tx_pad.

doc: correct exclude patterns after moving CMSIS files to cpu/sam21_c…

ebeece1

…ommon

cpu/sam21_common: eliminate trailing white space in CMSIS include files

1b307b3

dist/tools: exclude sam21_common cmsis include files

bd2fbf0

cpu/sam21_common: add C++ wrapper to sam0.h

e118525

cpu/sam21_common: remove define of LITTLE_ENDIAN

d367090

This define conflicts with LITTLE_ENDIAN defined in include/machine/endian.h which is part of gcc-arm-none-eabi. Also, the define does not seem to be used by the ASF included files.

keestux force-pushed the refactor-samd21 branch from d2bcafe to d367090 Compare September 21, 2016 18:59

kaspar030 approved these changes Sep 22, 2016

View reviewed changes

kaspar030 merged commit 0e45604 into RIOT-OS:master Sep 22, 2016

This was referenced Sep 22, 2016

cpu/sam21_common: support SAMR21E18 #5861

Merged

samd21: spi: fix power regression #5868

Merged

keestux deleted the refactor-samd21 branch September 30, 2016 18:29

haukepetersen mentioned this pull request Oct 5, 2016

cpu/sam21: removed superfluous gpio_init_sercom #5910

Merged

dylad mentioned this pull request Jan 9, 2017

SAM L21 update to ASF xdk 3.33 #6242

Merged

keestux mentioned this pull request Apr 13, 2017

SAMD21: possible CMSIS bug ? #6874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor samd21 #5743

Refactor samd21 #5743

keestux commented Aug 10, 2016

PeterKietzmann commented Aug 11, 2016

kaspar030 commented Aug 11, 2016

kaspar030 Aug 11, 2016

keestux Aug 11, 2016

kaspar030 Aug 11, 2016 via email

keestux Aug 12, 2016

jnohlgard commented Aug 11, 2016 •

edited

Loading

kaspar030 commented Aug 11, 2016

kaspar030 commented Aug 11, 2016

keestux commented Aug 11, 2016

kaspar030 commented Aug 13, 2016

jnohlgard commented Aug 13, 2016

kaspar030 commented Aug 13, 2016 •

edited

Loading

jnohlgard commented Aug 13, 2016

kaspar030 commented Aug 13, 2016

PeterKietzmann commented Aug 15, 2016

kaspar030 commented Aug 15, 2016

keestux commented Aug 15, 2016

keestux commented Aug 15, 2016

kYc0o commented Aug 19, 2016

keestux commented Sep 21, 2016

kaspar030 commented Sep 22, 2016

immesys commented Sep 22, 2016

haukepetersen commented Oct 5, 2016

keestux commented Oct 6, 2016

miri64 commented Oct 7, 2016

immesys commented Oct 7, 2016 •

edited

Loading

OlegHahm commented Oct 12, 2016

Refactor samd21 #5743

Refactor samd21 #5743

Conversation

keestux commented Aug 10, 2016

PeterKietzmann commented Aug 11, 2016

kaspar030 commented Aug 11, 2016

kaspar030 Aug 11, 2016

Choose a reason for hiding this comment

keestux Aug 11, 2016

Choose a reason for hiding this comment

kaspar030 Aug 11, 2016 via email

Choose a reason for hiding this comment

keestux Aug 12, 2016

Choose a reason for hiding this comment

jnohlgard commented Aug 11, 2016 • edited Loading

kaspar030 commented Aug 11, 2016

kaspar030 commented Aug 11, 2016

keestux commented Aug 11, 2016

kaspar030 commented Aug 13, 2016

jnohlgard commented Aug 13, 2016

kaspar030 commented Aug 13, 2016 • edited Loading

jnohlgard commented Aug 13, 2016

kaspar030 commented Aug 13, 2016

PeterKietzmann commented Aug 15, 2016

kaspar030 commented Aug 15, 2016

keestux commented Aug 15, 2016

keestux commented Aug 15, 2016

kYc0o commented Aug 19, 2016

keestux commented Sep 21, 2016

kaspar030 commented Sep 22, 2016

immesys commented Sep 22, 2016

haukepetersen commented Oct 5, 2016

keestux commented Oct 6, 2016

miri64 commented Oct 7, 2016

immesys commented Oct 7, 2016 • edited Loading

OlegHahm commented Oct 12, 2016

jnohlgard commented Aug 11, 2016 •

edited

Loading

kaspar030 commented Aug 13, 2016 •

edited

Loading

immesys commented Oct 7, 2016 •

edited

Loading