Remove &dyn references from GPIO-related HILs and capsules. #1765

gendx · 2020-04-17T10:38:27Z

Pull Request Overview

This pull request experiments with removing &dyn references from GPIO-related capsules, as a supporting example for #1761.

Until now, the capsules were using dynamic references to use GPIO pins, even though for a given board/chip the GPIOs have a type known at compile time. This pull request proposes to replace such dynamic references with explicit types.

The resulting code is a bit more verbose, but surprisingly not so much. Most of the extra code comes from components, where static_init_half is now necessary to instantiate the static types that are now generic.

By applying this refactoring to the most common capsules (GPIO, button, LED), savings of the order of ~1KB can be obtained on the boards (depending on how many GPIOs are declared by the board). More savings could be obtained by extending this refactoring to all capsules using GPIOs. And more generally many HILs in the kernel rely on &dyn references, so removing them from HILs and capsules in general could achieve even bigger savings.

Before:

   text	   data	    bss	    dec	    hex	filename
 102400	   3196	  62336	 167932	  28ffc	/tock/target/thumbv7em-none-eabi/release/hail
 164864	   3272	  54072	 222208	  36400	/tock/target/thumbv7em-none-eabi/release/imix
  68609	   1952	  79968	 150529	  24c01	/tock/target/thumbv7em-none-eabi/release/nucleo_f429zi

After:

   text	   data	    bss	    dec	    hex	filename
 101888	   3196	  62336	 167420	  28dfc	/tock/target/thumbv7em-none-eabi/release/hail
   -512       =       =            -200
 163840	   3272	  54072	 221184	  36000	/tock/target/thumbv7em-none-eabi/release/imix
  -1024       =       =            -400
  66561	   1952	  79968	 148481	  24401	/tock/target/thumbv7em-none-eabi/release/nucleo_f429zi
  -2048       =       =            -800

Testing Strategy

This pull request was tested by building a few boards and checking the binary size. Not much more testing at this stage.

TODO or Help Wanted

This pull request still needs:

Feedback about such a revamp of the HIL.
Feedback about whether reducing code size by this method is useful (see also [Tracking] Optimizing Tock for code size #1663).
Feedback about the components.
Updating the documentation.
Testing on physical boards.

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make formatall.

gendx · 2020-04-17T11:00:36Z

Note: it seems that these capsules used to be parameterized by the GPIO pin type before the last revamp of the HIL (#1297). However:

At that time, dynamic references were not explicitly marked as such in the code, so this aspect might have been overlooked.
The HIL redesign also changed many other things.
I here propose some code size measurements that show some gains, and code size is of interest at least for some Tock users ([Tracking] Optimizing Tock for code size #1663).

bradjc

Note: it seems that these capsules used to be parameterized by the GPIO pin type before the last revamp of the HIL (#1297).

Yes that is what I remember, and since we have generic components either way is fine to me. The dyn approach is cleaner, but the parametrized approach is more in-line with other capsules. If there is some other benefit one way or the other then great.

ppannuto

That's a pretty significant improvement, which make sense given the context for the optimizations that can be made with less dynamic dispatch.

I agree that the "visual overhead" is not too bad here, but I really hate the Rust convention of single-letter types (or double, IP is good). I feel as these stack up they really impede the readability of code; though maybe it's something that can be addressed with creative UI in IDEs. The rest of Tock goes to lengths to spell out identifiers and make things readable; I don't really think we should do anything differently here, just.. venting into the void a bit.

gendx · 2020-04-20T08:33:27Z

Awesome!

Before I extend this to the other boards, any feedback on the components changes? I moved the GPIO pins from the finalize() to the new() part, but maybe that's not the intended use (I'm still confused by the semantics of new vs. finalize)?

ppannuto · 2020-04-21T17:35:10Z

Before I extend this to the other boards, any feedback on the components changes? I moved the GPIO pins from the finalize() to the new() part, but maybe that's not the intended use (I'm still confused by the semantics of new vs. finalize)?

🤷 it looks fine to me -- then again, I'm also in support of collapsing those into one function as proposed in #1618. Maybe @bradjc has opinions.

bradjc · 2020-04-30T22:10:36Z

Awesome!

Before I extend this to the other boards, any feedback on the components changes? I moved the GPIO pins from the finalize() to the new() part, but maybe that's not the intended use (I'm still confused by the semantics of new vs. finalize)?

Looks good to me.

ppannuto · 2020-05-01T17:10:48Z

This does technically touch a HIL, so it probably qualifies as significant. It's also been in the review queue for a while though, so I think we can move reasonably quickly on this once updated and fully implemented.

hudson-ayers

I agree that the design looks good, and the savings are convincing. I think this just needs to be ported to other boards and rebased.

… imix, hail, nucleo_f429zi.

gendx · 2020-05-05T16:32:00Z

Rebased and migrated all boards. Where is the size report summary supposed to show?

hudson-ayers · 2020-05-05T16:33:37Z

Click "details" next to the "benchmarks" check, then click the "size report" step. (I included your change to print the git diff there, but it has made the report a little cluttered).

gendx · 2020-05-05T16:37:56Z

Click "details" next to the "benchmarks" check, then click the "size report" step. (I included your change to print the git diff there, but it has made the report a little cluttered).

Ok, thanks. I thought there was some pipeline to show the results directly on the pull request page.

hudson-ayers · 2020-05-05T16:40:09Z

Click "details" next to the "benchmarks" check, then click the "size report" step. (I included your change to print the git diff there, but it has made the report a little cluttered).

Ok, thanks. I thought there was some pipeline to show the results directly on the pull request page.

That pipeline currently only works for PRs submitted from branches on the upstream repo, due to limitations with how Github allows commit status posting. A plan is in the works to get around this limitation by hosting part of the CI on a local server where secrets can safely be stored.

ppannuto

bors d+

This is great! I've confirmed that buttons still happily button on Hail.

I think this is ready to go unless you have any remaining questions on the size improvements Hudson? That conversation looked mechanistic, but I didn't want to prematurely merge.

bors · 2020-05-05T16:45:52Z

✌️ gendx can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

hudson-ayers

bors r+

bors · 2020-05-05T16:59:19Z

Canceled.

gendx · 2020-05-05T17:00:23Z

I updated the usage of affected components as well. Will wait a day in case anyone has something to say.

alistair23 · 2020-05-05T17:38:10Z

This PR is still marked as RFC. Is it no longer an RFC?

gendx · 2020-05-06T13:41:23Z

This PR is still marked as RFC. Is it no longer an RFC?

Indeed.

gendx · 2020-05-06T13:48:48Z

bors r+

bors · 2020-05-06T13:59:58Z

Build succeeded:

bors · 2020-05-06T14:00:08Z

Build succeeded:

This was referenced Apr 17, 2020

[Tracking] Optimizing Tock for code size #1663

Open

[RFC] Simpler and more robust component interface #1618

Closed

bradjc previously approved these changes Apr 17, 2020

View reviewed changes

ppannuto previously approved these changes Apr 20, 2020

View reviewed changes

bradjc added the needs-rebase label Apr 30, 2020

ppannuto added HIL This affects a Tock HIL interface. P-Significant This is a substancial change that requires review from all core developers. labels May 1, 2020

hudson-ayers previously approved these changes May 5, 2020

View reviewed changes

Remove &dyn references for some GPIO-related capsules and apply it to…

129f992

… imix, hail, nucleo_f429zi.

gendx dismissed stale reviews from hudson-ayers, ppannuto, and bradjc via 129f992 May 5, 2020 15:06

gendx force-pushed the nodyn-gpio branch from f38a79f to 129f992 Compare May 5, 2020 15:06

gendx added 3 commits May 5, 2020 17:28

Migrate GPIOs for stm32f3discovery and arty_e21 boards.

0ecd180

Migrate GPIOs for launchxl, nucleo_f446re, opentitan boards.

82019b4

Migrate GPIOS for Nordic boards.

33b9c20

gendx requested review from hudson-ayers, bradjc and ppannuto May 5, 2020 16:31

ppannuto previously approved these changes May 5, 2020

View reviewed changes

hudson-ayers previously approved these changes May 5, 2020

View reviewed changes

Update usage of GPIO-related components.

0401f78

gendx dismissed stale reviews from hudson-ayers and ppannuto via 0401f78 May 5, 2020 16:59

gendx requested review from hudson-ayers and ppannuto May 5, 2020 17:00

hudson-ayers approved these changes May 5, 2020

View reviewed changes

gendx changed the title ~~[RFC] Remove &dyn references from GPIO-related HILs and capsules.~~ Remove &dyn references from GPIO-related HILs and capsules. May 6, 2020

bors bot merged commit 8a65899 into tock:master May 6, 2020

gendx deleted the nodyn-gpio branch May 6, 2020 14:01

gendx mentioned this pull request May 18, 2020

kernel: add a config switch for process debug info #1830

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove &dyn references from GPIO-related HILs and capsules. #1765

Remove &dyn references from GPIO-related HILs and capsules. #1765

gendx commented Apr 17, 2020

gendx commented Apr 17, 2020

bradjc left a comment

ppannuto left a comment

gendx commented Apr 20, 2020

ppannuto commented Apr 21, 2020

bradjc commented Apr 30, 2020

ppannuto commented May 1, 2020

hudson-ayers left a comment

gendx commented May 5, 2020

hudson-ayers commented May 5, 2020

gendx commented May 5, 2020 •

edited

hudson-ayers commented May 5, 2020

ppannuto left a comment

bors bot commented May 5, 2020

hudson-ayers left a comment

bors bot commented May 5, 2020

gendx commented May 5, 2020

alistair23 commented May 5, 2020

gendx commented May 6, 2020

gendx commented May 6, 2020

bors bot commented May 6, 2020

bors bot commented May 6, 2020

Remove &dyn references from GPIO-related HILs and capsules. #1765

Remove &dyn references from GPIO-related HILs and capsules. #1765

Conversation

gendx commented Apr 17, 2020

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

gendx commented Apr 17, 2020

bradjc left a comment

Choose a reason for hiding this comment

ppannuto left a comment

Choose a reason for hiding this comment

gendx commented Apr 20, 2020

ppannuto commented Apr 21, 2020

bradjc commented Apr 30, 2020

ppannuto commented May 1, 2020

hudson-ayers left a comment

Choose a reason for hiding this comment

gendx commented May 5, 2020

hudson-ayers commented May 5, 2020

gendx commented May 5, 2020 • edited

hudson-ayers commented May 5, 2020

ppannuto left a comment

Choose a reason for hiding this comment

bors bot commented May 5, 2020

hudson-ayers left a comment

Choose a reason for hiding this comment

bors bot commented May 5, 2020

gendx commented May 5, 2020

alistair23 commented May 5, 2020

gendx commented May 6, 2020

gendx commented May 6, 2020

bors bot commented May 6, 2020

bors bot commented May 6, 2020

gendx commented May 5, 2020 •

edited