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

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

Closed
wants to merge 1 commit into from

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