Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boards/common/arduino-atmega: fix issue with wrong port for LED0 #18245

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

krzysztof-cabaj
Copy link
Contributor

Contribution description

valentinpi in Issue #18228 reports BUG with arduino UNO and NANO LED0.

Investigation of this bug reveals two independent problems:

  1. There is a wrong port for LED0_ON, OFF and TOGGLE in board_common.h for atmega328p and atmega2560 - this PR fix this issue.
  2. There are still some problems with periph_gpio for atmega. In current RIOT master example/blinky did not work. However, when 2022.04-branch is used with fix from this PR everything works fine. Initial workaround for this problem could be merging this commit directly to 2022.04-branch, and temporary using it for atmega.

Testing procedure

Compile and flash example/blinky for arduino uno or mega using current RIOT master - LED0 did not blink.

Switch to 2022.04-branch, apply this PR, compile and flash example/blinky for arduino uno or mega - LED0 should blink.

Issues/PRs references

Issue #18228

@github-actions github-actions bot added the Area: boards Area: Board ports label Jun 23, 2022
@krzysztof-cabaj krzysztof-cabaj changed the title boards/common/adruino-atmega: fix issue with wrong port for LED0 boards/common/arduino-atmega: fix issue with wrong port for LED0 Jun 23, 2022
@benpicco benpicco requested review from maribu and kfessel June 23, 2022 08:58
@maribu
Copy link
Member

maribu commented Jun 23, 2022

Aha! And I always thought the LED on my cheap clones was broken / misplaced :)

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Thanks for the fix!

@kfessel
Copy link
Contributor

kfessel commented Jun 23, 2022

seem strange to me (first look) to put non common defines into \*_common ,this is of cause no reason to stop this since it fixes an issue

@krzysztof-cabaj
Copy link
Contributor Author

Thanks for rapid service!

@maribu
Copy link
Member

maribu commented Jun 23, 2022

seem strange to me (first look) to put non common defines into \*_common ,this is of cause no reason to stop this since it fixes an issue

I totally agree (both)!

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 23, 2022
@maribu maribu enabled auto-merge June 23, 2022 13:21
@maribu
Copy link
Member

maribu commented Jun 23, 2022

Thanks for rapid service!

Thanks for the fix! :)

@maribu maribu merged commit f17cb33 into RIOT-OS:master Jun 24, 2022
@krzysztof-cabaj
Copy link
Contributor Author

Can we add this commit to the 2022.04-branch? Should I make new PR directly into that branch?

Currently examples/blinky did not work for arduino UNO and MEGA both in master nor 2022.04-branch. However, when this commit is added to 2022.04-branch everything works. Of course this is only workaround before problem with periph/gpio will be fixed.

@maribu
Copy link
Member

maribu commented Jun 25, 2022

This could be backported, however, it does work in master (just tested with an Seeduino arduino-uno clone with exmaples/blinky). Maybe you forgot to pull from upstream into your local branch?

What is the periph/gpio problem you are referring to?

@krzysztof-cabaj
Copy link
Contributor Author

I checked once again, and with this commit, with my arduino-uno and arduino-mega clones examples/blinky did not work.
It compile and flash without problem, I could connect to console, see banner but LED did not blinks, When I cherry picked this commit to 2022.04-branch - everything works perfect. When I earlier investigate this bug I noticed that some gpio functions are changed between 2022.04-branch and master - but I have no time to investigate this with details. I assumed that some recent commits introduce a bug.

@valentinpi could you test this PR on your board?

@maribu
Copy link
Member

maribu commented Jun 25, 2022

The last change in periph_gpio in cpu/atmega_common that actually touches code related to writing to GPIOs is from November 2019 ... What I believe you saw in the lock is that ATmegas already support a new alternative GPIO API, periph/gpio_ll. That is however not yet used anywhere in RIOT expect for two test applications and it does not touch the existing implementation. And since that is not used in examples/blinky, it cannot cause any issues.

I just tried again with BUILD_IN_DOCKER=1 and that also works. For reference, I checked with 32a1c22 being the HEAD of master.

@krzysztof-cabaj
Copy link
Contributor Author

krzysztof-cabaj commented Jun 26, 2022

Very strange situation. I double-checked everything and still I have different effects for master and 2022.04-branch code. Master - HEAD -> 32a1c22, 2022.04-branch - initially HEAD -> 657adb1 with cherry-picked 1e8c798. In such environment examples/blinky compiled from master did not blink LED at my arduino-uno. Code from 2022.04-branch works perfectly. What is interesting, when I looked at disassembly for gpio_init it differs (details below). This fact (I realize this when I work on fix for LED error) let my assume that there are some changes in periph_gpio. Maybe the reason is my old compiler avr-gcc: avr-gcc (GCC) 5.4.0? In the following week I test this at my University laboratory with docker configuration.
Any other hints?

Code compiled from 2022.04-branch - this works (6 bytes/3 instructions smaller).

00000116 <gpio_init>:
     116:	98 2f       	mov	r25, r24
     118:	9f 70       	andi	r25, 0x0F	; 15
     11a:	21 e0       	ldi	r18, 0x01	; 1
     11c:	30 e0       	ldi	r19, 0x00	; 0
     11e:	01 c0       	rjmp	.+2      	; 0x122 <gpio_init+0xc>
     120:	22 0f       	add	r18, r18
     122:	9a 95       	dec	r25
     124:	ea f7       	brpl	.-6      	; 0x120 <gpio_init+0xa>
     126:	62 30       	cpi	r22, 0x02	; 2
     128:	d9 f0       	breq	.+54     	; 0x160 <gpio_init+0x4a>
     12a:	63 30       	cpi	r22, 0x03	; 3
     12c:	79 f0       	breq	.+30     	; 0x14c <gpio_init+0x36>
     12e:	61 11       	cpse	r22, r1
     130:	28 c0       	rjmp	.+80     	; 0x182 <gpio_init+0x6c>
     132:	82 95       	swap	r24
     134:	8f 70       	andi	r24, 0x0F	; 15
     136:	93 e0       	ldi	r25, 0x03	; 3
     138:	89 9f       	mul	r24, r25
     13a:	f0 01       	movw	r30, r0
     13c:	11 24       	eor	r1, r1
     13e:	81 a1       	ldd	r24, Z+33	; 0x21
     140:	20 95       	com	r18
     142:	82 23       	and	r24, r18
     144:	81 a3       	std	Z+33, r24	; 0x21
     146:	82 a1       	ldd	r24, Z+34	; 0x22
     148:	28 23       	and	r18, r24
     14a:	17 c0       	rjmp	.+46     	; 0x17a <gpio_init+0x64>
     14c:	82 95       	swap	r24
     14e:	8f 70       	andi	r24, 0x0F	; 15
     150:	93 e0       	ldi	r25, 0x03	; 3
     152:	89 9f       	mul	r24, r25
     154:	f0 01       	movw	r30, r0
     156:	11 24       	eor	r1, r1
     158:	81 a1       	ldd	r24, Z+33	; 0x21
     15a:	28 2b       	or	r18, r24
     15c:	21 a3       	std	Z+33, r18	; 0x21
     15e:	0e c0       	rjmp	.+28     	; 0x17c <gpio_init+0x66>
     160:	82 95       	swap	r24
     162:	8f 70       	andi	r24, 0x0F	; 15
     164:	93 e0       	ldi	r25, 0x03	; 3
     166:	89 9f       	mul	r24, r25
     168:	f0 01       	movw	r30, r0
     16a:	11 24       	eor	r1, r1
     16c:	81 a1       	ldd	r24, Z+33	; 0x21
     16e:	92 2f       	mov	r25, r18
     170:	90 95       	com	r25
     172:	89 23       	and	r24, r25
     174:	81 a3       	std	Z+33, r24	; 0x21
     176:	82 a1       	ldd	r24, Z+34	; 0x22
     178:	28 2b       	or	r18, r24
     17a:	22 a3       	std	Z+34, r18	; 0x22
     17c:	80 e0       	ldi	r24, 0x00	; 0
     17e:	90 e0       	ldi	r25, 0x00	; 0
     180:	08 95       	ret
     182:	8f ef       	ldi	r24, 0xFF	; 255
     184:	9f ef       	ldi	r25, 0xFF	; 255
     186:	08 95       	ret

Code compiled from master

00000116 <gpio_init>:
     116:	98 2f       	mov	r25, r24
     118:	9f 70       	andi	r25, 0x0F	; 15
     11a:	21 e0       	ldi	r18, 0x01	; 1
     11c:	30 e0       	ldi	r19, 0x00	; 0
     11e:	01 c0       	rjmp	.+2      	; 0x122 <gpio_init+0xc>
     120:	22 0f       	add	r18, r18
     122:	9a 95       	dec	r25
     124:	ea f7       	brpl	.-6      	; 0x120 <gpio_init+0xa>
     126:	62 30       	cpi	r22, 0x02	; 2
     128:	d9 f0       	breq	.+54     	; 0x160 <gpio_init+0x4a>
     12a:	63 30       	cpi	r22, 0x03	; 3
     12c:	89 f0       	breq	.+34     	; 0x150 <gpio_init+0x3a>
     12e:	61 11       	cpse	r22, r1
     130:	2b c0       	rjmp	.+86     	; 0x188 <gpio_init+0x72>
     132:	93 e0       	ldi	r25, 0x03	; 3
     134:	89 9f       	mul	r24, r25
     136:	f0 01       	movw	r30, r0
     138:	11 24       	eor	r1, r1
     13a:	df 01       	movw	r26, r30
     13c:	90 96       	adiw	r26, 0x20	; 32
     13e:	81 a1       	ldd	r24, Z+33	; 0x21
     140:	20 95       	com	r18
     142:	82 23       	and	r24, r18
     144:	81 a3       	std	Z+33, r24	; 0x21
     146:	12 96       	adiw	r26, 0x02	; 2
     148:	8c 91       	ld	r24, X
     14a:	12 97       	sbiw	r26, 0x02	; 2
     14c:	28 23       	and	r18, r24
     14e:	17 c0       	rjmp	.+46     	; 0x17e <gpio_init+0x68>
     150:	93 e0       	ldi	r25, 0x03	; 3
     152:	89 9f       	mul	r24, r25
     154:	f0 01       	movw	r30, r0
     156:	11 24       	eor	r1, r1
     158:	81 a1       	ldd	r24, Z+33	; 0x21
     15a:	28 2b       	or	r18, r24
     15c:	21 a3       	std	Z+33, r18	; 0x21
     15e:	11 c0       	rjmp	.+34     	; 0x182 <gpio_init+0x6c>
     160:	93 e0       	ldi	r25, 0x03	; 3
     162:	89 9f       	mul	r24, r25
     164:	f0 01       	movw	r30, r0
     166:	11 24       	eor	r1, r1
     168:	df 01       	movw	r26, r30
     16a:	90 96       	adiw	r26, 0x20	; 32
     16c:	81 a1       	ldd	r24, Z+33	; 0x21
     16e:	92 2f       	mov	r25, r18
     170:	90 95       	com	r25
     172:	89 23       	and	r24, r25
     174:	81 a3       	std	Z+33, r24	; 0x21
     176:	12 96       	adiw	r26, 0x02	; 2
     178:	8c 91       	ld	r24, X
     17a:	12 97       	sbiw	r26, 0x02	; 2
     17c:	28 2b       	or	r18, r24
     17e:	12 96       	adiw	r26, 0x02	; 2
     180:	2c 93       	st	X, r18
     182:	80 e0       	ldi	r24, 0x00	; 0
     184:	90 e0       	ldi	r25, 0x00	; 0
     186:	08 95       	ret
     188:	8f ef       	ldi	r24, 0xFF	; 255
     18a:	9f ef       	ldi	r25, 0xFF	; 255
     18c:	08 95       	ret

@maribu
Copy link
Member

maribu commented Jun 26, 2022

That's strange. I could not see any change to gpio_init() in the relevant time frame with git log --patch cpu/atmega_common/periph/gpio.c, but I am currently afk and, hence, cannot double check. The changes I recall were only relevant to gpio_init_int(), which shouldn't cause this issue.

Can you delete the bin folder between builds (or run make clean)? The reason is that the build system doesn't support incremental builds when the source tree changes (which certainly is a shame...).

Could you also try with BUILD_IN_DOCKER=1, if cleaning the build didn't help?

@valentinpi
Copy link

valentinpi commented Jun 26, 2022

Well this is... Interesting. Testing out the following very simple firmware with my Arduino Nano:

#include <ztimer.h>

int main(void) {
    while (1) {
        ztimer_sleep(ZTIMER_SEC, 1);
    }

    return 0;
}
APPLICATION = beepy
RIOTBASE ?= ../RIOT

BOARD ?= arduino-nano

MAKEFLAGS += -j
DEVELHELP ?= 1
QUIET ?= 1

# See https://github.com/qmk/qmk_firmware/issues/17064
CFLAGS += -Wno-array-bounds

USEMODULE += ztimer
USEMODULE += ztimer_sec

include $(RIOTBASE)/Makefile.include

Flashing via:

$ make clean all
$ avrdude -c arduino -b 115200 -p m328p -P /dev/ttyUSB0 -U flash:w:<...>/beepy/bin/arduino-nano/beepy.hex
$ make term

Yields a stack overflow:

2022-06-26 20:13:30,678 # main(): This is RIOT! (Version: 2022.07-devel-895-gdd574)
2022-06-26 20:13:31,781 # scheduler(): stack overflow detected, pid=1

This worked when I reported the problem. Regarding the LED: It is still not working for some strange reason. I have not tested the fix on my Arduino Uno. Also, I still cannot ping my small beeper. What I mean by that is that the following firmware does not function as expected:

#include <periph/gpio.h>

int main(void) {
    const gpio_t SPKR_PIN = GPIO_PIN(1, 1);
    gpio_init(SPKR_PIN, GPIO_OUT);
    gpio_set(SPKR_PIN);

    while (1) {
    }

    return 0;
}

Makefile and flashing commands as above. The beeper is connected to the pin D9, which is PB1. Maybe for some background: I just wanted to make a very small application that has a proximity sensor and this beeper, and when something gets close to it the beeper should beep. But the GPIO is not working as I expect it to.

Regarding the parts: The beeper and the LED are both working when they are bridged to the 3.3V connection, so I suspect the HW is fine.

@maribu
Copy link
Member

maribu commented Jun 26, 2022

Could you peovide the Makefile as well, so that I can exactly reproduce?

Is the RIOT welcome message printed, or does it get stuck during boot?

@valentinpi
Copy link

valentinpi commented Jun 26, 2022

Oh I provided the makefile. It is for both firmwares. The RIOT welcome message gets printed, I appended a small log.

@krzysztof-cabaj
Copy link
Contributor Author

Can you delete the bin folder between builds (or run make clean)? The reason is that the build system doesn't support incremental builds when the source tree changes (which certainly is a shame...).

Yes, I clean manually bin during my test.

Could you also try with BUILD_IN_DOCKER=1, if cleaning the build didn't help?

I try to do this further in the week.

@krzysztof-cabaj
Copy link
Contributor Author

Also, I still cannot ping my small beeper. What I mean by that is that the following firmware does not function as expected:

#include <periph/gpio.h>

int main(void) {
    const gpio_t SPKR_PIN = GPIO_PIN(1, 1);
    gpio_init(SPKR_PIN, GPIO_OUT);
    gpio_set(SPKR_PIN);

    while (1) {
    }

    return 0;
}

Makefile and flashing commands as above. The beeper is connected to the pin D9, which is PB1. Maybe for some background: I just wanted to make a very small application that has a proximity sensor and this beeper, and when something gets close to it the beeper should beep. But the GPIO is not working as I expect it to.

Did you test it using most recent master branch? Could you switch to 2022.04-branch?
I suspect that this change (#18245 (comment)) in gpio_init do not correctly initialize ports.

@maribu
Copy link
Member

maribu commented Jun 27, 2022

I just ran elf_diff on the output for examples/blinky for the arduino-uno for master and 2022.04-branch: It shows that gpio_init has grown by 6 bytes, but the instructions remain unchanged. The C code of gpio_init is also unchanged, including the code of the helper functions it calls. Note that I compiled with RIOT_CI_BUILD=1, which results in less noise during compilation and most importantly sets the RIOT version string to some constant (I think test?) - otherwise a different version string would result in different binaries even if the C code would be identical.

I can reproduce the detected stack overflow and will investigate. I think the issue is that AVR doesn't have an ISR stack and the timer ISR victimizes the stack of the idle thread, which is too small.

Unrelated to the issue at hand, but still worth pointing out: Using ZTIMER_SEC for sleeping one second is not ideal, as ztimer_sleep() will sleep for at least the specified number +/- 1 tick. (At least because when the system is busy with higher priority tasks, it will have to attend them first.) So if you want to sleep a second, you should better use ZTIMER_MSEC in order to be off by +/- one ms, rather than by +/- one second.

@maribu
Copy link
Member

maribu commented Jun 27, 2022

I also can reproduce the issue with PB1 not being set in the application from #18245 (comment) while it does work with the latest stable branch. I was able to reproduce with both an Arduino UNO as well with a custom ATmega1284P board that has a JTAG connector, so that debugging is hopefully relatively easy.

@valentinpi
Copy link

@krzysztof-cabaj I tested the application with the master branch, but not with the other one. However, @maribu's commit fixes the issue, as well as the problem with the idle stack size. Thanks a lot! :)

@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
@krzysztof-cabaj krzysztof-cabaj deleted the arduino-atmega-LED0 branch November 23, 2022 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants