Skip to content

cpu/atmega_common: Cleanup state flags#13308

Merged
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
maribu:atmega_state_cleanup
Feb 10, 2020
Merged

cpu/atmega_common: Cleanup state flags#13308
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
maribu:atmega_state_cleanup

Conversation

@maribu
Copy link
Member

@maribu maribu commented Feb 6, 2020

Contribution description

  • Use one byte of RAM to track both IRQ and UART TX state
  • Fix incorrect use of volatile

Result of elf_diff on my machine: -1 Byte RAM, + 42 Bytes ROM

Testing procedure

TX should still work as expected.

Issues/PRs references

#12973

- Use one byte of RAM to track both IRQ and UART TX state
- Fix incorrect use of volatile
@maribu maribu added 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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Feb 6, 2020
@maribu maribu requested review from aabadie and fjmolinas February 6, 2020 14:43
Comment on lines +175 to +177
unsigned long state = irq_disable();
atmega_state |= ATMEGA_STATE_FLAG_UART_TX(uart);
irq_restore(state);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the point I expect to increase the ROM. The issue is that the AVR cannot set a flag in SRAM atomically. The original version would have looked like:

lds r24, _tx_pending
or r24, r25
sts _tx_pending, r24

Or in my own words:

  1. Load _tx_pending into a register
  2. Set a bit in the register (according to the value in uart)
  3. Write the register back into _tx_pending

Lets say in _tx_pending the bit for UART1 is set, and for UART0 is not. Now command 1. is executed and this state is loaded into the register. If the IRQ kicks in and clears the bit in _tx_pending this update is lost, as command 3 will restore this bit. Therefore this whole "load, set bit, store" sequence needs to be done atomically. The AVR cannot do this in a single instruction, so we have to resort to disabling interrupts. (Or using C11 atomic, which would internally do the same but with more overhead.)

@maribu maribu force-pushed the atmega_state_cleanup branch from e328feb to 1879f58 Compare February 6, 2020 15:01
@maribu
Copy link
Member Author

maribu commented Feb 6, 2020

Btw: The reason I stumbled upon this is that putting the AVR into sleep mode (for saving power or reducing noise during ADC conversions) the UART peripheral is just turned off; even if the last character is not fully send out. Having int atmega_is_uart_tx_pending(void) in place makes it easy to address this issue on other places as well.

@fjmolinas
Copy link
Contributor

Nice, will test asap.

@fjmolinas
Copy link
Contributor

TX should still work as expected.

BOARD=arduino-uno make -C tests/sys_arduino flash test
2020-02-06 16:10:38,637 # Help: Press s to start test, r to print it is ready
2020-02-06 16:10:38,772 # READY
s
2020-02-06 16:10:38,834 # START
2020-02-06 16:10:38,907 # main(): This is RIOT! (Version: 2020.04-devel-287-g1879f5-pr-13308)
2020-02-06 16:10:38,924 # Hello Arduino!
wrang
2020-02-06 16:10:39,427 # UNK
echo quite long string echoing on arduino module test
2020-02-06 16:10:39,984 # ECHO: quite long string echoing on arduino module test
numb 4242
2020-02-06 16:10:40,505 # 4242 4242 1092 10222
time
2020-02-06 16:10:41,103 # 2421768 2463304 2505720 OK END
print
2020-02-06 16:10:41,639 # print(int, BIN): 1111101011000111
2020-02-06 16:10:41,676 # println(int, BIN): 1111101011000111
2020-02-06 16:10:41,705 # print(int, OCT): 175307
2020-02-06 16:10:41,733 # println(int, OCT): 175307
2020-02-06 16:10:41,758 # print(int, DEC): -1337
2020-02-06 16:10:41,783 # println(int, DEC): -1337
2020-02-06 16:10:41,807 # print(int, HEX): fac7
2020-02-06 16:10:41,836 # println(int, HEX): fac7
2020-02-06 16:10:41,869 # print(unsigned int, BIN): 101010
2020-02-06 16:10:41,906 # println(unsigned int, BIN): 101010
2020-02-06 16:10:41,938 # print(unsigned int, OCT): 52
2020-02-06 16:10:41,971 # println(unsigned int, OCT): 52
2020-02-06 16:10:42,004 # print(unsigned int, DEC): 42
2020-02-06 16:10:42,036 # println(unsigned int, DEC): 42
2020-02-06 16:10:42,065 # print(unsigned int, HEX): 2a
2020-02-06 16:10:42,102 # println(unsigned int, HEX): 2a
2020-02-06 16:10:42,155 # print(long, BIN): 10110110011010011111110100101110
2020-02-06 16:10:42,209 # println(long, BIN): 10110110011010011111110100101110
2020-02-06 16:10:42,241 # print(long, OCT): 26632376456
2020-02-06 16:10:42,278 # println(long, OCT): 26632376456
2020-02-06 16:10:42,311 # print(long, DEC): -1234567890
2020-02-06 16:10:42,344 # println(long, DEC): -1234567890
2020-02-06 16:10:42,372 # print(long, HEX): b669fd2e
2020-02-06 16:10:42,405 # println(long, HEX): b669fd2e
2020-02-06 16:10:42,467 # print(unsigned long, BIN): 1001001100101100000001011010010
2020-02-06 16:10:42,532 # println(unsigned long, BIN): 1001001100101100000001011010010
2020-02-06 16:10:42,573 # print(unsigned long, OCT): 11145401322
2020-02-06 16:10:42,618 # println(unsigned long, OCT): 11145401322
2020-02-06 16:10:42,655 # print(unsigned long, DEC): 1234567890
2020-02-06 16:10:42,700 # println(unsigned long, DEC): 1234567890
2020-02-06 16:10:42,737 # print(unsigned long, HEX): 499602d2
2020-02-06 16:10:42,778 # println(unsigned long, HEX): 499602d2

@fjmolinas
Copy link
Contributor

I don't get the exact same diff, but it is in the same general lines, I guess it depends on the exact application, less RAM, more ROM.

BUILD_IN_DOCKER=1 RIOT_CI_BUILD=1 BOARD=arduino-uno make -C tests/sys_arduino -j3

  • pr
   text	   data	    bss	    dec	    hex	filename
  11414	    464	   1192	  13070	   330e	/data/riotbuild/riotbase/tests/sys_arduino/bin/arduino-uno/tests_sys_arduino.elf
  • master
   text	   data	    bss	    dec	    hex	filename
  11296	    468	   1193	  12957	   329d	/data/riotbuild/riotbase/tests/sys_arduino/bin/arduino-uno/tests_sys_arduino.elf

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 6, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK.

@maribu
Copy link
Member Author

maribu commented Feb 10, 2020

@fjmolinas: All green here.

Note: There is also another use case for the state flags introduced here: It could also track if SPI, I2C, PWM or other stuff is in use that prevents going to low power modes. And adding some power management to AVR is overdue :-)

@fjmolinas
Copy link
Contributor

GO!

@fjmolinas fjmolinas merged commit e2dc125 into RIOT-OS:master Feb 10, 2020
@fjmolinas
Copy link
Contributor

Note: There is also another use case for the state flags introduced here: It could also track if SPI, I2C, PWM or other stuff is in use that prevents going to low power modes. And adding some power management to AVR is overdue :-)

this could interest @roberthartung

@roberthartung
Copy link
Member

@fjmolinas thanks for the mention. My current implementation ( #8207 ) relies on pm_block() in pm_layered. I will try to work on a rebase and update on #8207 asap.

@leandrolanzieri leandrolanzieri added the Area: cpu Area: CPU/MCU ports label Feb 20, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020
@maribu maribu deleted the atmega_state_cleanup branch May 11, 2020 13:34
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants