atmega: refactor cpu/board code and build/flash variables by kYc0o · Pull Request #9130 · RIOT-OS/RIOT

kYc0o · 2018-05-15T00:59:27Z

Contribution description

This PR proposes a refactor of the atmega based boards to match the code reuse done in other architectures. In summary:

Added atmega.inc.mk for toolchain variables
Added avrdude.inc.mk for flashing variables
Move cpu.c and startup.c to cpu/atmega_common
Share cpu_conf.h to unify the stack memory definitions
Cleanup all unnecessary code

I hope I factored out as much as possible, if you find something else that can be reused please let me know.

Issues/PRs references

kYc0o · 2018-05-15T01:01:33Z

@Josar I'm not sure if you included arduino-common because you really need it or just to import toolchain and flasher requirements. Can you confirm?

bergzand · 2018-05-15T08:22:30Z

Thanks for picking this up @kYc0o. Is it possible to split this PR into 2 (or more) PR's? One with the makefile refactor and one with the cpu code refactor?

Josar · 2018-05-15T09:50:30Z

cpu/atmega_common/cpu.c

@@ -28,6 +33,17 @@
 #define ENABLE_DEBUG (0)


#include <avr/pgmspace.h> #include "cpu.h"

missing new line?

Josar · 2018-05-15T09:54:36Z

@kYc0o if i remember it correctly it was requested to minimize code duplicates.
Maybe @shr70 can comment on this.
But i think you are right it is just for the toolchain and flasher.
This was still something which i was not very happy with as the jiminy is not related to arduino.

aabadie

Found 2 minor things

boards/common/arduino-atmega/Makefile.include

cpu/atmega_common/Makefile.include

shr70 · 2018-05-15T13:11:59Z

@kYc0o @Josar Yes, we only included the arduino-common due to the flasher and build commands. This was requested in the review.

kYc0o · 2018-05-15T15:31:31Z

Update on this:

Solved murdock complaints about missing cpuid_get() on jiminy board
Removed dependency on arduino-atmega for jiminy board
Addressed comments of reviewers

@Josar or @shr70, can you test on jiminy if everything still works? Especially flashing.
@ZetaR60 Can you also test flashing on mega-xplained? I'm afraid I need to rewrite the flashing rules when not using a serial programmer.

@bergzand, I don't mind to decompose this PR in several, but if someone here can review it as is, it would be more practical.

Josar · 2018-05-15T16:40:36Z

@kYc0o flashing seems to work.

May i ask you to also change this depency to arduino?

RIOT/boards/jiminy-mega256rfr2/Makefile.features

Line 1 in d6c6359

include $(RIOTBOARD)/common/arduino-atmega/Makefile.features

I would suggest just add all FEATURES_PROVIDED to the jiminy Makefile.features. Or do i miss something which makes this a bad idea?

kaspar030 · 2018-05-15T19:56:22Z

+161 −791

I like!

kYc0o · 2018-05-15T21:15:01Z

Another update, and I think I'm triggering a more larger discussion.

Currently, atmega based boards are "forced" to import the CPU features from arduino-atmega/Makefile.features, which in my opinion doesn't make sense since 3 atmega based
boards are not arduinos, thus shouldn't have nothing to do with it.

I moved the features to cpu/atmega_common/Makefile.features where I thing they suit better, actually they are implemented in the same tree, which IMHO it's clear to understand why the features are defined there as well.

Finally, I left the arduino feature where I think it belongs, arduino-atmega/Makefile.features.

If nobody objects, I'd propose to start a "cleanup" in the future of these definitions, since currently every single board define features which are implemented at the cpu level.

Additionally, I removed the grouping needed previously by the CI, and if I'm not mistaken, Murdock doesn't need it anymore (@kaspar030 please correct me if I'm wrong).

ZetaR60

I like the refactor! Did a proofread with some suggested changes, and also noted where behavior has changed.

This currently does not work on mega-xplained, due to problems with PROGRAMMER_SPEED.

boards/arduino-duemilanove/Makefile.include

boards/mega-xplained/Makefile.include

boards/waspmote-pro/Makefile.include

makefiles/tools/avrdude.inc.mk

cpu/atmega_common/cpu.c

cpu/atmega256rfr2/include/cpu_conf.h

cpu/atmega_common/include/cpu_conf.h

cpu/atmega1281/startup.c

cpu/atmega328p/startup.c

kaspar030 · 2018-05-15T21:36:04Z

If nobody objects, I'd propose to start a "cleanup" in the future of these definitions, since currently every single board define features which are implemented at the cpu level.

Some features depend on cpu support and board configuration (periph_conf.h). That's why e.g., periph_uart is usually in a board's Makefile.features, even though the CPU provides the underlying implementation.

kaspar030 · 2018-05-15T21:41:04Z

I think @bergzand is right, this PR is getting very large. Difficult to spot problems.

ZetaR60 · 2018-05-15T21:42:08Z

I'm not sure about moving Makefile.features entries to a common area. How do you handle, for instance, a board that has all of the ADC pins connected to LED outputs? That board would not provide ADC support, even though the CPU supports it.

EDIT: kaspar030 was just slightly faster than me on this comment. ;)

kYc0o · 2018-05-15T22:01:31Z

My logic behind the features is that they might be provided by the CPU, but might not be used by the board. I think it's manageable to make a difference between both and express it in the code. Actually, shouldn't my change break some compiles? I was experimenting with the change expecting Murdock to complain about it.

kYc0o · 2018-05-22T17:04:08Z

What's next then on this PR? Should I squash to try a split in different PRs? What about the features? Should they stay at the board level or at the CPU level?

mali · 2018-05-23T10:22:35Z

tested with arduino Uno/Duemilanove boards 👍

kYc0o · 2018-05-23T15:24:21Z

@mali thanks for testing! Any thoughts about the overall cleanup?

mali · 2018-05-23T16:12:53Z

@kYc0o I haven't looked in details, but looks cleaner and more logical in general and well ... +215 -843 :-) I've also noticed that some .hex are a little smaller than before, while others are little bigger, but we talk about an handful of bytes...

smlng

minor (but blocking) issue ... otherwise looks good, tested with meg2560

smlng · 2019-01-07T16:06:11Z

makefiles/arch/atmega.inc.mk

+endif
+
+# the atmel port uses uart_stdio
+USEMODULE += uart_stdio


typo: this is actually named stdio_uart

The module was actually renamed in d55616a7f56ca17440 (so after this PR was opened) so not sure this really is a typo ;-)

You're both right, it's not a typo but it should be renamed. Thanks for spotting this!

I'll update asap.

smlng · 2019-01-07T16:42:46Z

to me this is good, please squash and rebase on latest master (had an issue which was solved in master while testing)

smlng · 2019-01-08T08:03:00Z

@ZetaR60 and @aabadie are we good to go here?

miri64 · 2019-01-08T08:11:03Z

I guess #7306 needs another rework if this is merged?

smlng · 2019-01-08T08:14:32Z

I guess #7306 needs another rework if this is merged?

might be, but rather minimal then - I think only boards/arduino-leonardo/Makefile.include would need some adaption, actually removing code (I think) ... but maybe @kYc0o can guide/help there

kYc0o · 2019-01-14T17:34:59Z

Ping @aabadie @ZetaR60

kYc0o · 2019-01-21T11:03:52Z

Weekly ping @aabadie @ZetaR60. I guess it didn't make it for the release?

smlng · 2019-01-22T07:48:58Z

please rebase

ZetaR60 · 2019-01-23T21:17:22Z

Sorry for the delay. The issues from my review seem to be resolved. Once it is rebased I will test on mega-xplained and arduino-uno.

maribu · 2019-02-03T18:33:02Z

@kYc0o: ping

Allows to use avrdude as a flashing tool in any context (e.g. not dependent on arduino or atmega) though it only works (AFAIK) on atmega, but I thought it's better to have it here as we have other flashing tools.

Everything is now defined in atmega.inc.mk, following the common RIOT-like reusability of rules and variables (e.g. cortexm.inc.mk).

Leverages common flasher (avrdude) and removes unnecessary exports. Moreover, a reuse of serial.inc.mk is perfomed from the same atmega_common/Makefile.include

Additionally, it removes unnecessary exports and cleans up waspmote-pro toolchain variables (not needed) which are taken from atmega_common.

Features must be provided by the board if they're actually available on board. Other features might be provided by the CPU. Some grouping is also removed as it is not necessary.

kYc0o · 2019-02-05T16:18:26Z

Rebased.

maribu · 2019-02-05T19:42:12Z

@ZetaR60: ping

(Sorry for making all the fuzz. I waiting for this PR to get merged in order to move two existing PRs forward and create a third on. Normally I'm less annoying ;-))

ZetaR60 · 2019-02-08T23:08:55Z

Final double-check on mega-xplained looks good. Thanks for the cleanup @kYc0o !

kYc0o · 2019-02-10T22:39:20Z

Thanks everyone for your review! 🎉

kYc0o added this to the Release 2018.07 milestone May 15, 2018

kYc0o requested review from bergzand and cladmi May 15, 2018 00:59

Josar reviewed May 15, 2018

View reviewed changes

aabadie requested changes May 15, 2018

View reviewed changes

boards/common/arduino-atmega/Makefile.include Outdated Show resolved Hide resolved

cpu/atmega_common/Makefile.include Outdated Show resolved Hide resolved

kYc0o force-pushed the cpu/atmega/unify_stacks branch from fd5cc94 to 2720b68 Compare May 15, 2018 15:25

ZetaR60 suggested changes May 15, 2018

View reviewed changes

kYc0o force-pushed the cpu/atmega/unify_stacks branch 2 times, most recently from 511c742 to 02cfde4 Compare May 25, 2018 15:31

smlng suggested changes Jan 7, 2019

View reviewed changes

kYc0o force-pushed the cpu/atmega/unify_stacks branch from f6e20b0 to c31f65f Compare January 7, 2019 16:45

kYc0o mentioned this pull request Jan 7, 2019

cpu/atmega_common: refactor AVR libc code into module #9984

Merged

smlng approved these changes Jan 8, 2019

View reviewed changes

aabadie approved these changes Jan 21, 2019

View reviewed changes

aabadie modified the milestones: Release 2019.01, Release 2019.04 Jan 25, 2019

kYc0o added 6 commits February 5, 2019 17:12

makefiles: add atmega arch generic makefile

c897773

makefiles: factor out avrdude configuration

6bbf372

Allows to use avrdude as a flashing tool in any context (e.g. not dependent on arduino or atmega) though it only works (AFAIK) on atmega, but I thought it's better to have it here as we have other flashing tools.

cpu/atmega*: make use of common atmega.inc.mk and remove redundancies

2bdcddd

Everything is now defined in atmega.inc.mk, following the common RIOT-like reusability of rules and variables (e.g. cortexm.inc.mk).

boards/arduino*: use common avrdude.inc.mk from atmega_common

1a98d64

Leverages common flasher (avrdude) and removes unnecessary exports. Moreover, a reuse of serial.inc.mk is perfomed from the same atmega_common/Makefile.include

boards: leverage avrdude.inc.mk for atmega based boards.

d208cba

Additionally, it removes unnecessary exports and cleans up waspmote-pro toolchain variables (not needed) which are taken from atmega_common.

boards: include potential features from CPU for atmega boards

7139393

Features must be provided by the board if they're actually available on board. Other features might be provided by the CPU. Some grouping is also removed as it is not necessary.

kYc0o force-pushed the cpu/atmega/unify_stacks branch from c31f65f to 7139393 Compare February 5, 2019 16:17

ZetaR60 approved these changes Feb 8, 2019

View reviewed changes

ZetaR60 merged commit 1f9e185 into RIOT-OS:master Feb 8, 2019

kYc0o deleted the cpu/atmega/unify_stacks branch February 10, 2019 22:38

Conversation

kYc0o commented May 15, 2018

Contribution description

Issues/PRs references

Uh oh!

kYc0o commented May 15, 2018

Uh oh!

bergzand commented May 15, 2018

Uh oh!

Josar May 15, 2018

Choose a reason for hiding this comment

Uh oh!

Josar commented May 15, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shr70 commented May 15, 2018

Uh oh!

kYc0o commented May 15, 2018

Uh oh!

Josar commented May 15, 2018

Uh oh!

kaspar030 commented May 15, 2018

Uh oh!

kYc0o commented May 15, 2018

Uh oh!

ZetaR60 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaspar030 commented May 15, 2018

Uh oh!

kaspar030 commented May 15, 2018

Uh oh!

ZetaR60 commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kYc0o commented May 15, 2018

Uh oh!

kYc0o commented May 22, 2018

Uh oh!

mali commented May 23, 2018

Uh oh!

kYc0o commented May 23, 2018

Uh oh!

mali commented May 23, 2018

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

smlng Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

kYc0o Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

smlng commented Jan 7, 2019

Uh oh!

smlng commented Jan 8, 2019

Uh oh!

miri64 commented Jan 8, 2019

Uh oh!

smlng commented Jan 8, 2019

Uh oh!

kYc0o commented Jan 14, 2019

ZetaR60 commented May 15, 2018 •

edited

Loading