Progress of port to SODAQ Autonomo

Despite the ongoing discussion of how cpu/sam* should be configured I am continuing with my effort to port RIOT to SODAQ Autonomo.

The changes are available in my add-sodaq-autonomo branch at git@github.com:keestux/RIOT.git

UART and I2C are working now, and so is xtimer. Next on my list is SPI.

For I2C there are a few changes in gpio that make the code simpler and configuration easier to understand. (At least, that's what I think :slight_smile:

Coding. While going through the code I notice that there are too many "magic" constants. Hard coded numbers that are obvious for some, but not obvious for others. My advise: always try to use defines and add a comment about what constants mean. Or point to datasheet sections explaining the constants.

One example (not to blame, but to hopefully improve in the future).

cpu/samd21/cpu.c:      /* redirect all peripherals to a disabled clock generator (7) by default */      for (int i = 0x3; i <= 0x22; i++) { What is that 0x22? It probably is SAMR21's GCLK_PTC, the last non-reserved Generic Clock Selection ID. BTW, the last non-reserved ID for SAMD21 is 0x24 which is GCLK_I2S_1. This particular should probable disable all peripherals, even the reserved ones. But the point is, the number 0x22 should explained.

Hi Kees, *,

While going through the code I notice that there are too many "magic" constants. Hard coded numbers that are obvious for some, but not obvious for others. My advise: always try to use defines and add a comment about what constants mean. Or point to datasheet sections explaining the constants.

Thank you for bringing this up.

I am uncertain if there is something that can be done about it in the existing code base, but at least we should find a way to prevent such issues in the future:

- Is there something in the (if, then probably Linux) coding conventions already? - if not, is anyone opposing the addition of such a rule? - if we have/want such a rule, can/should we add a static rule check for this in addition to the rule itself?

I support adding such a rule of it does not exist yet. In my experience the false positives are heavily outweighed by true magic numbers of the kind we want to avoid. Therefore, for both the rule and the static rule check I would strongly recommend it.

Cheers, Ludwig

There is an "immediately" missing in the sentence above... I guess there's quite a few of these :wink:

Cheers, Ludwig

All,

I am a little concerned the coding conventions getting too pedantic lately.

While I agree magic numbers should be avoided, I disagree to introduce #defines for every single one of them as I do with introducing a static check for it. (Using a #define for a number only used once in HW initialization for example seems too much to me.)

I agree however with putting a comment with an explanation or data sheet reference to each occurrence. (Register initializations should use or'ed bit defines from vendor headers anyways.) To me it is the duty of the maintainer reviewing the code to decide if it makes sense to use a #define or a comment is enough. Thus I'd prefer a best practice instead of a rule.

Best, Thomas