Skip to content

cpu/{atxmega,atmega_common}: fix invalid use of PSTR()#16920

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
maribu:cpu/avr8_common/bugfix
Oct 1, 2021
Merged

cpu/{atxmega,atmega_common}: fix invalid use of PSTR()#16920
benpicco merged 1 commit intoRIOT-OS:masterfrom
maribu:cpu/avr8_common/bugfix

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Sep 30, 2021

Contribution description

core_panic() doesn't expect the message to be in program memory, but
in data memory. Bad things will happen on AVR when the address is
interpreted as being in data address space, but the allocation is
done in program address space.

Testing procedure

The corresponding panics (and IRQ without provided ISR) should print something sensible again.

Issues/PRs references

None

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Platform: AVR Platform: This PR/issue effects AVR-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch labels Sep 30, 2021
@maribu maribu requested a review from benpicco September 30, 2021 13:28
@maribu maribu requested a review from kYc0o as a code owner September 30, 2021 13:28
@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 Sep 30, 2021
@github-actions github-actions bot added the Area: cpu Area: CPU/MCU ports label Sep 30, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Sep 30, 2021

Looks like quite a few applications are now out of RAM, since the error message is placed into .data now. Maybe being a bit less verbose in the message can fix the issue.

core_panic() doesn't expect the message to be in program memory, but
in data memory. Bad things will happen on AVR when the address is
interpreted as being in data address space, but the allocation is
done in program address space.
@maribu maribu force-pushed the cpu/avr8_common/bugfix branch from 832369b to 35a1b60 Compare September 30, 2021 15:15
@benpicco
Copy link
Copy Markdown
Contributor

That makes me wonder, how much would be needed to have functions that print fixed strings from PROGMEM?

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Sep 30, 2021

The avrlibc has special printf format specifier for that, or using puts_P() instead of puts(). The issue is that a char * pointing to progmem can have the same numeric pointer value as one to data memory, so one cannot dispatch correctly at run time

@benpicco benpicco 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 Oct 1, 2021
@benpicco benpicco merged commit badadd1 into RIOT-OS:master Oct 1, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 1, 2021

Backport provided in #16933

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 1, 2021

Thanks!

@maribu maribu deleted the cpu/avr8_common/bugfix branch October 1, 2021 19:55
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Platform: AVR Platform: This PR/issue effects AVR-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants