cpu/atmega*: factorise common code into atmega_common #9866

kYc0o · 2018-08-30T12:19:27Z

Contribution description

This PR aims to take some commits from #9130 to ease the review.
In this PR I took code from atmega platforms and place it in atmega_common, since it was basically all the same.

Testing procedure

Please run at least one example/test for each of these platforms:

Issues/PRs references

#9130

A-Paul

Running tests/{od,shell,thread_basic,thread_flood,xtimer_usleep} is successful.
But tests/xtimer_hang hangs (nomen est omen) before boot message. Crosschecked with master, which is fine.

ZetaR60

A few things below. I haven't tested yet, but it is next on my todo list.

cpu/atmega_common/cpu.c

Josar · 2018-09-02T20:31:53Z

Thumps up for generalizing code for all platforms.

kYc0o · 2018-09-03T14:37:52Z

I just pushed some modifications to generalise a bit more the code, however some registers don't exists for some platforms. @Josar and @ZetaR60 can you test on your atmega boards? So far some tests are ok with the mega2560 and uno.

@A-Paul I'll check this strange behaviour for xtimer.

kYc0o · 2018-09-03T16:55:41Z

@kaspar030 murdock seems frozen...

kYc0o · 2018-09-05T14:37:08Z

Rebased since #9781 was merged. @A-Paul I did the tests you mentioned and they run successfully on arduino-mega2560. Which platform are you using?

A-Paul · 2018-09-06T12:10:27Z

@kYc0o , in your list of boards I dedicated myself to test the arduino-duemilanove. ;-)

Josar · 2018-09-06T15:39:09Z

@kYc0o run this tests on jiminy with expected behaviour.

tests_xtimer_usleep_short
tests_xtimer_usleep
tests_xtimer_reset
tests_xtimer_remove
tests_xtimer_periodic_wakeup
tests_xtimer_hang
tests_thread_priority_inversion
tests_thread_race

kYc0o · 2018-09-06T15:42:03Z

@kYc0o , in your list of boards I dedicated myself to test the arduino-duemilanove. ;-)

Unfortunately @A-Paul I don't have that platform. Do you mind to test with the most recent commits? I don't see how that specific test can be affected by this PR.

A-Paul · 2018-09-06T15:45:29Z

After checking out your last changes I get compile errors for some tests. I only tried tests/xtimer_*.
I concatenated the make output (error lines only) to this gist:

https://gist.github.com/A-Paul/62c3838be10840386451d5a57b655824#file-gistfile1-txt

This happened to me with arduino-uno and arduino-duemilanove.

kYc0o · 2018-09-06T15:49:19Z

@kYc0o run this tests on jiminy with expected behaviour.

Thanks @Josar, can you tick your board?

After checking out your last changes I get compile errors for some tests.

Yes that's normal since #9781, which added linker scripts for AVR platforms, so now they will fail if the memory doesn't fit. Maybe this PR is increasing the size for the given tests? Anyways Murdock should have complained about it, and it doesn't.

kYc0o · 2018-09-06T15:53:06Z

Anyways Murdock should have complained about it, and it doesn't.

Overlooked the errors on Murdock, it does appear there. I'll check why there's a huge memory increase by this PR.

kYc0o · 2018-09-06T16:11:57Z

Well I found the reason, which is basically that having a common THREAD_STACKSIZE_DEFAULT per architecture increased the stack for all CPUs, and atmega328p is on the limit of RAM for many examples. Thus, 256B more is enough to overflow the RAM. This is also true for some tests with other atmegas.

My 2c is that I blacklist these platforms from this tests since running tests with less than 256B of memory left may lead to errors.

Any opinions?

ZetaR60

Test results on mega-xplained:

shell			still reboot loops!
xtimer_hang		fail (misses more than 10%)

thread_basic		pass
thread_flood		pass
thread_race		pass
xtimer_periodic_wakeup	pass
xtimer_remove		pass
xtimer_reset		pass
xtimer_usleep		pass
xtimer_usleep_short	pass

cpu/atmega_common/cpu.c

ZetaR60 · 2018-09-06T20:22:04Z

cpu/atmega_common/cpu.c

-ISR(BADISR_vect){
-
+ISR(BADISR_vect)
+{
    _reset_cause();

    printf_P(PSTR("FATAL ERROR: BADISR_vect called, unprocessed Interrupt.\n"
                  "STOP Execution.\n"));


This error message could probably be a core_panic with reason PANIC_DUMMY_HANDLER.

It would need to go where the while (1) {} is, because a panic halts the system.

Ok, I'll modify that part and make the corresponding tests.

I was thinking, is there an easy way to trigger this? So we can make a proper test.

cpu/atmega_common/cpu.c

ZetaR60 · 2018-09-06T20:47:27Z

I think that blacklisting the ATmega328P is fine because it is already known that ATmega has problems with the stack size being too small. ATmega328P is pretty limited compared to the typical RIOT MCU, and we probably don't want to keep the more capable systems broken in order to accommodate it.

I think a more comprehensive change to how stack sizes are determined is needed to properly support the ATmega328P.

A-Paul · 2018-09-10T13:17:35Z

sure why. Anyone else tried reboot using the shell?

@ZetaR60, I checked tests/shell with this PR on an arduino-duemilanove and arduino-mega2560.
With both boards:
Reset button: OK, make reset: OK, > reboot : loop
When looping neither reseting with button or make reset will change that state. Only reconnecting the board will.

This effect seems to be fairly common. Just an example:
https://andreasrohner.at/posts/Electronics/How-to-make-the-Watchdog-Timer-work-on-an-Arduino-Pro-Mini-by-replacing-the-bootloader/

A-Paul · 2018-09-10T13:53:16Z

I think a more comprehensive change to how stack sizes are determined is needed to properly support the ATmega328P.

There is some 'magic' for handling constant data and strings to lower ram usage.
https://www.microchip.com/webdoc/AVRLibcReferenceManual/pgmspace.html
On the fly, I can figure out at least one occurrence (cpu/atmega_common/cpu.c:127) where this is used. I guess that has potential for saving ram.

Josar · 2018-09-11T10:48:09Z

I can not push to your repo so i will post it here.

cpu.c

/*
* Since atmega MCUs do not feature a software reset, the watchdog timer
* is being used. It will be set to the shortest time and then force a
* reset. Therefore the MCUSR register needs to be resetted as fast as
* possible. 
* Which means in the bootloader or in the following init0 if no bootloader is used.
* Bootloader resets watchdog and pass MCUSR in r2 (e.g. Optiboot) in order to pass
* information about the reset cause to the application.
* When no Bootloader is used the watchdog will be disabled in the init0 section.
* When a software reset was triggered, r3 will contain 0xAA. 
* In order to prevent changes to the values from the .init section, MCUSR and r3 
* are saved in the .init0 section
*/
uint8_t mcusr_mirror __attribute__((section(".noinit")));
uint8_t soft_rst __attribute__((section(".noinit")));
void get_mcusr(void) __attribute__((naked)) __attribute__((section(".init0")));

void get_mcusr(void)
{
    /* save soft reset flag set in reset routine */
    __asm__ __volatile__("mov %0, r3\n" : "=r" (soft_rst) :);
#ifdef BOOTLOADER_CLEARS_WATCHDOG_AND_PASSES_MCUSR
    /* save the reset flags passed from the bootloader */
    __asm__ __volatile__("mov %0, r2\n" : "=r" (mcusr_mirror) :);
#else
    /* save the reset flags */
#if defined(__AVR_ATmega8515__) || defined(__AVR_ATmega8535__) ||	\
    defined(__AVR_ATmega16__)   || defined(__AVR_ATmega162__) ||	\
    defined (__AVR_ATmega128__)
  mcusr_mirror = MCUCSR;
  MCUSR = 0;
#else
  mcusr_mirror = MCUSR;
  MCUSR = 0;
#endif
    wdt_disable();
#endif
}

board.h (jiminy)

/**
 * @name Indicate Watchdog cleared in bootloader an 
 * 
 * AVR CPUs need to reset the Watchdog as fast as possible.
 * This flag indicates that the watchdog is reseted in the bootloader
 * and that the MCUSR value is stored in register 2 (r2)
 * @{
 */
#define BOOTLOADER_CLEARS_WATCHDOG_AND_PASSES_MCUSR 1
/** @} */

kYc0o · 2018-09-19T12:05:06Z

@Josar thanks for your patch! I'll add it soon to make this ready. I'll also address your comments @ZetaR60. Sorry for the delay.

Josar · 2018-09-19T12:16:48Z

@kYc0o this patch is only tested for the atmega256rfr2. The other part has to be tested and verified.

kYc0o · 2018-09-19T12:49:08Z

@Josar yes sure, I'll make tests on the platforms I have.

ZetaR60

Just a couple minor suggestions, otherwise it looks good!

Thanks for your work on this kYc0o!

ZetaR60 · 2018-10-06T19:56:45Z

cpu/atmega_common/cpu.c

+    /* save the reset flags */
+#if defined(__AVR_ATmega8515__) || defined(__AVR_ATmega8535__) ||	\
+    defined(__AVR_ATmega16__)   || defined(__AVR_ATmega162__) ||	\
+    defined (__AVR_ATmega128__)


If this amounts to "the system has an MCUCSR register", then this complex ifdef can be changed to #ifdef MCUCSR.

cpu/atmega_common/cpu.c

ZetaR60 · 2018-10-06T21:47:49Z

I wrote a quick test case to generate a BADISR. It is pretty tightly linked to this PR so you might want to add it here, but I can create a separate PR if you want. (Sorry for the blocks of code; we need hide-able sections in comments).

tests/atmega_badisr/main.c

/*
 * Copyright (C) 2018 Acutam Automation, LLC
 *
 * This file is subject to the terms and conditions of the GNU Lesser
 * General Public License v2.1. See the file LICENSE in the top level
 * directory for more details.
 */

/**
 * @ingroup     tests
 * @{
 *
 * @file
 * @brief       Test for ATmega BADISR handling
 *
 * @author      Matthew Blue <matthew.blue.neuro@gmail.com>
 *
 * @}
 */

#include <avr/io.h>
#include <stdio.h>

int main(void)
{
    puts("ATmega BADISR test routine");

    /* Initialize timer 0 */
    TCCR0A = 0;
    TCCR0B = 0;
    TCNT0 = 0;

    /* Enable overflow interrupt without defining ISR */
#ifdef TOIE0
    TIMSK0 = (1 << TOIE0);
#else
    TIMSK0 = (1 << TOIE);
#endif

    /* Clear interrupt flags (oddly, by writing a 1) */
#ifdef TOV0
    TIFR0 = (1 << TOV0);
#else
    TIFR0 = (1 << TOV);
#endif

    /* Start the clock (will BADISR at overflow) */
    TCCR0B = (1 << CS00);

    /* Wait for BADISR */
    while (1) {}

    return 0;
}

tests/atmega_badisr/Makefile

include ../Makefile.tests_common

BOARD_WHITELIST += arduino-duemilanove \
                   arduino-mega2560 \
                   arduino-uno \
                   jiminy-mega256rfr2 \
                   mega-xplained \
                   waspmote-pro

TEST_ON_CI_WHITELIST += arduino-duemilanove \
                        arduino-mega2560 \
                        arduino-uno \
                        jiminy-mega256rfr2 \
                        mega-xplained \
                        waspmote-pro

include $(RIOTBASE)/Makefile.include

tests/atmega_badisr/tests/01-run.py

#!/usr/bin/env python3

# Copyright (C) 2018 Acutam Automation, LLC
#
# This file is subject to the terms and conditions of the GNU Lesser
# General Public License v2.1. See the file LICENSE in the top level
# directory for more details.

import sys
from testrunner import run


def testfunc(child):
    child.expect_exact("ATmega BADISR test routine")
    child.expect_exact("*** RIOT kernel panic:")
    child.expect_exact("*** halted.")

if __name__ == "__main__":
    sys.exit(run(testfunc))

Note that Makefile definitions for tests have changed since this PR was created. Without a rebase you will need to add this to the Makefile:

test:
        tests/01-run.py

And here are the results I get on mega-xplained:

user@lm:~/Desktop/RIOT/tests/atmega_badisr$ make BOARD=mega-xplained term
/home/user/Desktop/RIOT/makefiles/toolchain/gnu.inc.mk:18: objcopy not found. Hex file will not be created.
/bin/sh: 1: avr-ld: not found
/home/user/Desktop/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "9600"
No handlers could be found for logger "root"
2018-10-06 17:34:26,585 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2018-10-06 17:34:28,588 - INFO # 

2018-10-06 17:34:28,659 - INFO # main(): This is RIOT! (Version: 2016.03-devel-9848-gb45d51-6807c15f4413-cpu/atmega/reuse_common)
2018-10-06 17:34:28,689 - INFO # ATmega BADISR test routine
2018-10-06 17:34:28,788 - INFO # *** RIOT kernel panic:
2018-10-06 17:34:28,789 - INFO #    
2018-10-06 17:34:28,790 - INFO # 
2018-10-06 17:34:28,791 - INFO # *** halted.
2018-10-06 17:34:28,792 - INFO # 
2018-10-06 17:34:33,904 - INFO # Exiting Pyterm

Note that the message for core_panic is not being printed.

kYc0o · 2018-10-11T13:57:21Z

@ZetaR60 comments addressed. For your test, maybe you can push a PR based on this one, with the "waiting for PR" label set.

Note that the message for core_panic is not being printed.

Maybe you need to activate DEVELHELP in the test makefile to having it printed.

I hope @Josar will have time soon to get this finally merged. Thanks @ZetaR60 for your testing and review!

kYc0o · 2018-10-11T16:11:48Z

@PeterKietzmann can you perform a quick test on waspmote-pro? compiling and flashing tests/shell and perform reboot should be enough.

kYc0o · 2018-10-30T18:44:43Z

@Josar could you run the tests on your platform?

Josar · 2018-10-31T05:32:46Z

shell			no reboot loop
xtimer_hang		pass (hiccup, but passed)
thread_basic		pass
thread_flood		pass
thread_race		pass
xtimer_periodic_wakeup	pass
xtimer_remove		pass
xtimer_reset		pass
xtimer_usleep		pass
xtimer_usleep_short	pass

kYc0o · 2018-10-31T11:24:57Z

Great! so, @ZetaR60 you can ACK now. I'll take care of the overflowed tests to get Murdock pass.

ZetaR60 · 2018-11-01T06:03:39Z

Double checked the tests before ACK:

shell			no reboot loop
xtimer_hang		pass
thread_basic		pass
thread_flood		pass
thread_race		pass
xtimer_periodic_wakeup	pass
xtimer_remove		pass
xtimer_reset		pass
xtimer_usleep		pass
xtimer_usleep_short	pass

Removes duplicated code for atmega platforms. They were all basically the same, only with the exception of atmegarfr2, for which there is an #if statement to use the code in the same file.

Removes redundancy of code since all the boards were defining the same variables. Moreover, the stack sizes are unified per architecture, as required.

cpu.c and startup.c were redundant in most platforms, except for atmega256rfr2. The common code is now in cpu/atmega_common/cpu.c and cpu/atmega_common/startup.c. cpu_conf.h is also removed as it's now in cpu/atmega_common/include thus shared by all atmega based platforms.

This adds a LED_PANIC macro which defines which LED, or combination of LEDs should notify a panic error. This is currently used to signal BADISR_vect errors.

The `atmega_set_prescaler` was using a "sensible" default but it's better to define it at the board level to make it clear.

This variable helps to inform at boot time that some information about the booting process, e.g. reset cause.

The unification of a bigger stack for the atmega platforms makes some boards to not have enough memory to provide the big stack plus the application code. It is possible though, to override the stack size to a smaller amount if running the test is necessary.

ZetaR60 · 2018-11-04T21:45:48Z

Merging. Thanks for the cleanup @kYc0o !

kaspar030 · 2018-11-04T22:11:13Z

+215 -721! Nice one!

kYc0o added this to the Release 2018.10 milestone Aug 30, 2018

kYc0o force-pushed the cpu/atmega/reuse_common branch from d981050 to 6e93286 Compare August 30, 2018 12:36

kYc0o mentioned this pull request Aug 30, 2018

atmega: refactor cpu/board code and build/flash variables #9130

Merged

A-Paul previously requested changes Aug 30, 2018

View reviewed changes

ZetaR60 suggested changes Sep 2, 2018

View reviewed changes

cpu/atmega_common/cpu.c Outdated Show resolved Hide resolved

cpu/atmega_common/cpu.c Show resolved Hide resolved

kYc0o force-pushed the cpu/atmega/reuse_common branch from 047e25f to b399f3f Compare September 5, 2018 14:35

ZetaR60 suggested changes Sep 6, 2018

View reviewed changes

ZetaR60 suggested changes Oct 6, 2018

View reviewed changes

jia200x added this to To Do in 2018.10 Release Planning Oct 10, 2018

jia200x modified the milestones: Release 2018.10, Release 2019.01 Oct 25, 2018

ZetaR60 approved these changes Nov 1, 2018

View reviewed changes

kYc0o force-pushed the cpu/atmega/reuse_common branch from d895a49 to 5bbe73a Compare November 2, 2018 15:22

kYc0o added 5 commits November 2, 2018 16:23

cpu/atmega_common: add cpu.c and startup.c common code

feac98c

Removes duplicated code for atmega platforms. They were all basically the same, only with the exception of atmegarfr2, for which there is an #if statement to use the code in the same file.

cpu/atmega_common: use the same cpu_conf for all atmega based boards

4f28407

Removes redundancy of code since all the boards were defining the same variables. Moreover, the stack sizes are unified per architecture, as required.

boards: add LED_PANIC to atmega based boards

5823f69

This adds a LED_PANIC macro which defines which LED, or combination of LEDs should notify a panic error. This is currently used to signal BADISR_vect errors.

boards: add atmega based boards clock scale defaults

956ae52

The `atmega_set_prescaler` was using a "sensible" default but it's better to define it at the board level to make it clear.

kYc0o force-pushed the cpu/atmega/reuse_common branch from 5bbe73a to 029af75 Compare November 2, 2018 15:26

jiminy-mega256rfr2: add BOOTLOADER_CLEARS_WATCHDOG_AND_PASSES_MCUSR

73f6328

This variable helps to inform at boot time that some information about the booting process, e.g. reset cause.

kYc0o force-pushed the cpu/atmega/reuse_common branch from 029af75 to 65c531e Compare November 2, 2018 15:39

kYc0o force-pushed the cpu/atmega/reuse_common branch from 65c531e to dd3ca90 Compare November 2, 2018 16:57

ZetaR60 merged commit 8130874 into RIOT-OS:master Nov 4, 2018

This was referenced Nov 4, 2018

Issuing a reboot on ATmega328p makes the board loop-reboot #6836

Closed

examples/gnrc_minimal: don't work anymore for atmega #9001

Closed

boards/common/atmega: initialize watchdog #9141

Closed

kYc0o deleted the cpu/atmega/reuse_common branch November 5, 2018 00:45

cpu/atmega*: factorise common code into atmega_common #9866

cpu/atmega*: factorise common code into atmega_common #9866

Conversation

kYc0o commented Aug 30, 2018 • edited

Contribution description

Testing procedure

Issues/PRs references

A-Paul left a comment

Choose a reason for hiding this comment

ZetaR60 left a comment

Choose a reason for hiding this comment

Josar commented Sep 2, 2018

kYc0o commented Sep 3, 2018

kYc0o commented Sep 3, 2018

kYc0o commented Sep 5, 2018

A-Paul commented Sep 6, 2018

Josar commented Sep 6, 2018 • edited

kYc0o commented Sep 6, 2018

A-Paul commented Sep 6, 2018

kYc0o commented Sep 6, 2018

kYc0o commented Sep 6, 2018

kYc0o commented Sep 6, 2018 • edited

ZetaR60 left a comment

Choose a reason for hiding this comment

ZetaR60 Sep 6, 2018

Choose a reason for hiding this comment

kYc0o Sep 19, 2018

Choose a reason for hiding this comment

kYc0o Sep 21, 2018

Choose a reason for hiding this comment

ZetaR60 commented Sep 6, 2018

A-Paul commented Sep 10, 2018

A-Paul commented Sep 10, 2018

Josar commented Sep 11, 2018

kYc0o commented Sep 19, 2018

Josar commented Sep 19, 2018

kYc0o commented Sep 19, 2018

ZetaR60 left a comment

Choose a reason for hiding this comment

ZetaR60 Oct 6, 2018

Choose a reason for hiding this comment

ZetaR60 commented Oct 6, 2018 • edited by kYc0o

kYc0o commented Oct 11, 2018

kYc0o commented Oct 11, 2018

kYc0o commented Oct 30, 2018

Josar commented Oct 31, 2018 • edited

kYc0o commented Oct 31, 2018

ZetaR60 commented Nov 1, 2018

ZetaR60 commented Nov 4, 2018

kaspar030 commented Nov 4, 2018

kYc0o commented Aug 30, 2018 •

edited

Josar commented Sep 6, 2018 •

edited

kYc0o commented Sep 6, 2018 •

edited

ZetaR60 commented Oct 6, 2018 •

edited by kYc0o

Josar commented Oct 31, 2018 •

edited