Skip to content

drivers/at86rf2xx: don't copy params to RAM#12712

Closed
benpicco wants to merge 1 commit intoRIOT-OS:masterfrom
benpicco:at86rf2xx-const_params
Closed

drivers/at86rf2xx: don't copy params to RAM#12712
benpicco wants to merge 1 commit intoRIOT-OS:masterfrom
benpicco:at86rf2xx-const_params

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Nov 14, 2019

Contribution description

The params struct is not changed at run-time, so there is no point in copying it to RAM.

With the gnrc_networking on samr21-xpro the difference is rather negligible though:

This uses 44 bytes more ROM and 16 bytes less RAM.

before

before:
   text    data     bss     dec     hex
  91596     192   19060  110848   1b100

after

   text    data     bss     dec     hex
  91640     192   19044  110876   1b11c

Testing procedure

I ran gnrc_networking and did not notice a difference in behavior.

Issues/PRs references

Came up in #12641 when discussing if it's possible to find out the index of a netdev in the at86rf215_params[] array.

The params struct is not changed, so there is no point in copying
it to RAM.

This uses 44 bytes *more* ROM and 16 bytes *less* RAM.
@benpicco benpicco added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Nov 14, 2019
@benpicco benpicco requested review from maribu and smlng November 14, 2019 18:59
@maribu
Copy link
Member

maribu commented Nov 14, 2019

Some cons:

  • This will increase RAM on the ATmega128RFA1/ATmega256RFR2 (or generally any Harvard architecture)
  • It will slow down accesses a bit due to the additional level of indirection
  • It will slow down accesses on most von-Neumann architectures, as access to the flash is generally slower than access to RAM. (Though, the faster chips have a cache for the ROM to mitigate that somewhat.)

However: I have no strong opinion on this. Code-wise and documentation-wise this is objectively fine.

@maribu maribu added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Nov 14, 2019
@maribu
Copy link
Member

maribu commented Nov 14, 2019

I let others decide on whether the trade-off is worth it.

@benpicco
Copy link
Contributor Author

benpicco commented Nov 14, 2019

Yea I posted this more as a basis for discussion.
It's cleaner to not keep a needles copy around, but the added code size is hard to justify just for that.
It was more a proof of concept that the params index to eui64 index idea would work with just small changes to the code.

ATmega128RFA1/ATmega256RFR2 doesn't use that struct, but with arduino-mega2560 and tests/driver_at86rf2xx I get

before

   text	   data	    bss	    dec	    hex
  25352	   2086	   1851	  29289	   7269

after

   text	   data	    bss	    dec	    hex
  25444	   2086	   1846	  29376	   72c0	

So it's really not worth it.

@benpicco benpicco closed this Nov 17, 2019
@benpicco benpicco deleted the at86rf2xx-const_params branch September 15, 2021 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines 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.

2 participants