sys/shell: reduce overhead of XFA shell commands #20958
Open
+93
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maribu
added
Type: cleanup
|
Ah, I should also show that this does actually bring down This PR: And in |
Murdock results❌ FAILED 55a1a7a DROPME: fix build with rust
Build failures (1)
Artifacts |
|
Interesting: On |
maribu
force-pushed
the
sys/shell/xfa-reduce-overhead
branch
from
f4b2ef1
to
9434532
Compare
maribu
requested review from
miri64,
leandrolanzieri,
aabadie,
MichelRottleuthner,
kaspar030 and
bergzand
as code owners
github-actions
bot
added
Area: network
maribu
added
the
State: waiting for other PR
maribu
force-pushed
the
sys/shell/xfa-reduce-overhead
branch
from
9434532
to
c9240ac
Compare
github-actions
bot
added
Platform: ARM
maribu
added a commit
to maribu/rust-riot-wrappers
that referenced
this pull request
Nov 6, 2024
This was referenced Nov 6, 2024
maribu
added a commit
to maribu/rust-riot-wrappers
that referenced
this pull request
Nov 7, 2024
maribu
force-pushed
the
sys/shell/xfa-reduce-overhead
branch
from
c106c72
to
7ebf793
Compare
mguetschow
reviewed
Nov 7, 2024
Comment on lines
-286
to
+305
| XFA_USE_CONST(shell_command_xfa_t*, shell_commands_xfa); \ | ||
| XFA_USE_CONST(shell_command_xfa_t, shell_commands_xfa_v2); \ |
There was a problem hiding this comment.
Why do we need to rename this? I would have expected this name to be an implementation detail.
There was a problem hiding this comment.
It is an implementation detail, but the implementation of shell XFA is split across two repos: This repo contains the C implementation, https://github.com/RIOT-OS/rust-riot-wrappers contains the rust implementation.
If the rust impl would live here as well, I would have just adapted both in lock-step and kept the symbol name. But with the current state of affair, there is a benefit to be able to implement the rust side in a way that it will work with both variants of RIOT. (E.g. so that we can update the rust side while using a stable release on the C side.) See RIOT-OS/rust-riot-wrappers#134 for why this is beneficial for the rust side.
There was a problem hiding this comment.
Alright, thanks for the explanation, makes sense!
This command does the same as `help`, but provides a machine readable JSON rather than a human readable table. It is only provided when the (pseudo-)module `shell_json` is used.
This increases the robustness of the test by not relying on the order shell commands are printed in. At least for XFA based shell commands, there is no guarantee in which order they will be shown in the help.
We do not need to add an array of pointers to the shell commands, just an array of shell commands is sufficient. This reduced the overhead of XFA by `sizeof(void *)` per command.
maribu
force-pushed
the
sys/shell/xfa-reduce-overhead
branch
from
7ebf793
to
55a1a7a
Compare
github-actions
bot
added
Area: build system
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
We do not need to add an array of pointers to the shell commands, just an array of shell commands is sufficient. This reduced the overhead of XFA by
sizeof(void *)per command.Testing procedure
And
nativewill be tested by the CI.Issues/PRs references