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

general: find method to globally change shared data structures (shell commands, device parameters, network interfaces, ...) #4290

Closed
kaspar030 opened this issue Nov 17, 2015 · 17 comments
Assignees
Labels
Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: don't stale State: Tell state-bot to ignore this issue Type: new feature The issue requests / The PR implemements a new feature for RIOT

Comments

@kaspar030
Copy link
Contributor

We have many places where it would be really nice to add an entry to a shared array, from any file.

Think auto_init, shell_commands, device driver parameters, network interfaces, sensors, libc initializers, ...

I really appreciate that we haven't chosen to go libc's route of using the linker to sort these things out.
Still, I think that the current pile of hacks will get harder to maintain the more features RIOT gets.

I'm trying to come up with a nice way to be able to add entries to a global, shared array (per type).
I think all of the above can efficiently be modeled using const arrays of structs.

Something like:

in sys/random/sc_random.c:

SHELL_COMMAND { "random_init", function, ... }

in sys/gnrc/blah.c:

SHELL_COMMAND { "gnrc_print_blah", function, ... }

ends up as, e.g., bin/include/shell_commands_array.h:

shell_command_t shell_commands[] = {
  { "random_init", function, ... },
  { "gnrc_print_blah", function, ... },
}

IMHO, if we do this right, we can reuse it for all of the above "lists".
But:

  • C can't do it by itself
  • generated code sucks
  • ...

Opinions?

@kaspar030 kaspar030 added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Feature Request labels Nov 17, 2015
@OlegHahm
Copy link
Member

I guess linked lists are out of question because of memory overhead?

@kaspar030
Copy link
Contributor Author

I guess linked lists are out of question because of memory overhead?

Well, yes. That would require run-time "register shell_command" for stuff that's available at runtime.
Also, that wouldn't work for auto-init.

@OlegHahm
Copy link
Member

I'm not so sure I understand the auto_init problem. Where do we have arrays there?

@kaspar030
Copy link
Contributor Author

I'm not so sure I understand the auto_init problem. Where do we have arrays there?

Well, currently we manually call void() auto init functions in auto_init.c.
That's no "problem", but needs manual maintanance of that file.

But that could be implemented as an iteration over a list of void(*)(void) function pointers.
That way, any auto-init module could specify in it's main .c file something like,

AUTO_INIT &xtimer_auto_init

... using the same global-shared-array method as shell commands and coap handlers.

auto_init.c could stay unmodified, just including the generated header with the list of function pointers.
(auto-init would need some kind of ordering, though)

@kaspar030
Copy link
Contributor Author

Most project (e.g., Linux for modules) do this using linker sections. IMHO that gets clumsy and is limited (e.g., no configurable sorting). But it could be the way to go.

@haukepetersen
Copy link
Contributor

It would indeed be very very nice to have this, and it would simplify things in many places quite significantly!

When keeping to the standard toolchain tools, I think using the linker script might be our only choice. I don't worry so much about being clumsy and the sorting, I think the bigger problem to solve regarding linker scripts are the platforms, where we use default ld scripts and have no means to add sections to them...

@kaspar030
Copy link
Contributor Author

@haukepetersen While I love that you're able to change your mind, I hate that I listen to you too much. ;)

Agreed, linker scripts are the way to go. I'll close this.

@kaspar030
Copy link
Contributor Author

sorry, wron issue closed.

@haukepetersen
Copy link
Contributor

@kaspar030: changing mind about what? You mean about using linker script sections? I have not really changed my mind (yet), all I said is, that I see this as our only option if want to achieve what you described in this issue... I did not say anything about liking/disliking this myself :-)

@OlegHahm
Copy link
Member

OlegHahm commented Feb 9, 2016

Agreed, linker scripts are the way to go.

I'd like to register a "Hate-to-say-I-told-you-so"-callback.

@kaspar030
Copy link
Contributor Author

I'd like to register a "Hate-to-say-I-told-you-so"-callback.

Why? You were the second person basically saying "NO!" to anything linker script related.

Personally I prefer a file snippet based solution (as in #4598), but noone seems to care. I want any solution in, without it, so many easy things become sooo clumsy...

@OlegHahm
Copy link
Member

OlegHahm commented Feb 9, 2016

Why? You were the second person basically saying "NO!" to anything linker script related.

And I still am.

@OlegHahm OlegHahm modified the milestone: Release 2016.04 Mar 18, 2016
@miri64 miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT and removed Feature Request labels Sep 30, 2018
@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@miri64
Copy link
Member

miri64 commented Aug 10, 2019

@kaspar030 isn't this solved with #9105?

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@kaspar030
Copy link
Contributor Author

@kaspar030 isn't this solved with #9105?

yes, that's the latest attempt!

@stale
Copy link

stale bot commented Apr 18, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 18, 2020
@stale stale bot closed this as completed May 19, 2020
@kaspar030 kaspar030 added the State: don't stale State: Tell state-bot to ignore this issue label May 19, 2020
@kaspar030 kaspar030 reopened this May 19, 2020
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label May 19, 2020
@kaspar030
Copy link
Contributor Author

Solved by #15002.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: don't stale State: Tell state-bot to ignore this issue Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

No branches or pull requests

4 participants