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

doc/memos: RDM Runtime Configuration Architecture #10622

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Dec 17, 2018

Contribution description

This PR is meant to explain the current efforts and ideas on how to implement a runtime configuration system, and to discuss about the proposed architecture. This is one of the concerns of the Configuration Task Force.

If needed, further RDMs can be drafted to explain in detail the different parts of the system (RIOT Registry, storage facilities, configuration manager and interfaces).

You can see the output of the draft here

@leandrolanzieri leandrolanzieri added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR TF: Config Marks issues and PRs related to the work of the Configuration Task Force Area: RDM Area: Discussions on RIOT Developer Memos labels Dec 17, 2018
@jia200x jia200x added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Dec 17, 2018
@kb2ma
Copy link
Member

kb2ma commented Dec 28, 2018

Nice first pass at this! Conceptually the approach makes sense, and I agree that a facility to store configuration is useful. I do not have experience with trying to solve this problem though, so my comments below are more generic.

Context is a good place to start. What alternative implementations are available in other embedded systems? Is the design here in the mainstream? I see a link to Apache mynewt but no reference in the text.

Consider addition of a real example. I like the flow section, but the discussion would be more concrete with a specific example, like setting the IP address for a resource directory server, or maybe the group of configuration items for networking.

I like the bullets in section 4 for the top level interface to registry handlers. Can you do the same for the storage interface in section 5 and configuration manager in section 6? That would help focus their main responsibilities.

I was thrown off by the use of 'commit' in the registry section. That verb seems like it would apply to the storage section. Is there a better name for this notification that the registry is ready to use?

The diagram is really helpful. The size of configuration interface in the diagram makes it seem like it's more important than storage or the registry handlers. From the discussion, it seems like all three of these elements are important, with maybe storage a little less so because its purpose is pretty clear. Maybe resize the components to reflect their importance.

@jia200x
Copy link
Member

jia200x commented Jan 15, 2019

Re the quality/style of these RDMs in general - what are we going for? A couple of possibilities:

IETF RFCs
Rust RFCs
In particular, whether the language and content should stand up to scrutiny and analysis and be always explicit, or whether they are being offered as more general guides and the content/wording can be more implicit and general. I think it's worth discussing and deciding on this explicitly
@jia200x @leandrolanzieri @emmanuelsearch @kaspar030 @miri64 @tcschmidt any thoughts?

Can we at least move this discussion to an issue or add a PR to the RDM-0?

@danpetry
Copy link
Contributor

I've raised an issue, aiming to address style and review standards of documentation across the board: #10778

@danpetry
Copy link
Contributor

danpetry commented Jan 17, 2019

After discussion offline, here's some clarification of the process of these, from RDM0

Proposing an RDM is non-compulsory. There is no restriction on when, in the development cycle, an RDM can be PRed. However, RDMs on design decisions and architecture are typically expected to describe something rather stable (that is already merged in the main branch of RIOT, or about to be merged).

This means that an implementation PR can, and may often be, merged before a design RDM PR.

There seems to me to be some ambiguity around what a minor revision consists of, compared to when an RDM should be deprecated, which I've attempted to address here: #10798.

@jia200x
Copy link
Member

jia200x commented Jan 17, 2019

This means that an implementation PR can, and may often be, merged before a design RDM PR.

I would say it's nice to have an RDM before an implementation, because it helps gathering consensus and explaining design decisions.

There seems to me to be some ambiguity around what a minor revision consists of, compared to when an RDM should be deprecated, which I've attempted to address here: #10798.

IMO RDMs should focus more in "clarity" (help other developers understand what the motivations are, provide useful resources, explain why it's designed that way, etc) rather than "quality" (strict definitions, sharp scope, strict rules, etc). While I agree it's always better to have a perfect document, in practice there will be an overhead that doesn't necessarily reflect in a better implementation.

E.g in this document we are trying to explain why we think the architecture is suitable for RIOT and why we plan to integrate the RIOT Registry into RIOT. It might (or maybe not) be a good approach for Runtime Configuration System and that's why I think these kind of discussions should also be addressed here (so if this gets rejected, implementers don't waste time writing code that will never get merged)

Because of this, I would personally prefer to focus more on design issues with this document and evident problems (typos, design not clear enough, etc). After #10798 gets resolved we can add a new revision to this document (or deprecate).

@danpetry
Copy link
Contributor

danpetry commented Jan 17, 2019

#10798 has three ACKs, but I'll leave it open until tomorrow morning in case you want to make amendments. After it's merged, I'll review this PR following the guidelines contained in it.

@jia200x
Copy link
Member

jia200x commented Jan 17, 2019

#10798 has three ACKs, but I'll leave it open in case you want to make amendments. After it's merged, I'll review this PR by the guidelines contained in it.

I just merged it ;)

@danpetry
Copy link
Contributor

Ok, cool, thanks for that :)

Copy link
Member

@tcschmidt tcschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some detail ... plus some editorial style: It would be IMO helpful to introduce figure captions with numbering and refer to the figures from the text.

- A UART shell with special commands for interacting with configurations (CLI)

A Configuration Manager may interact with the RIOT Registry to access Runtime
Configurations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a "MAY" here? According to the first architecture graph, the Configuration Manager accesses the RIOT Registry.
It would be IMO helpful to clarify

  • which is the interface or surface the Configuration Manager sees and interacts with
  • which (southbound) API(s) does it need to implement towards RIOT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a "MAY" here? According to the first architecture graph, the Configuration Manager accesses the RIOT Registry.

You are right, the Configuration Managers should only access the parameters via the Registry.

It would be IMO helpful to clarify

  • which is the interface or surface the Configuration Manager sees and interacts with
  • which (southbound) API(s) does it need to implement towards RIOT

Each implemented Configuration Manager will interact directly with the RIOT Registry API.

Regarding the API of the Configuration Managers towards RIOT, I'm not sure if we extract a common interface it at the moment, as all of them are really different (IMO Configuration Managers could even require an RDM on their own). We can though define some responsibilities they have (e.g. access control mechanisms, enable/disable of interfaces). Maybe that can be rephrased in the document.

@tcschmidt
Copy link
Member

IMO the state of this document is quite advanced.
Regarding the previous lengthy meta-discussion: How far are we with the implementation? Is the state of the document already well reflected?

@leandrolanzieri
Copy link
Contributor Author

leandrolanzieri commented Jan 29, 2019

@tcschmidt :

How far are we with the implementation?

From the described architecture, implemented and working (though further testing is still needed):

  • The Registry. I'm still adding tests and some missing documentation so I can remove the WIP status.
  • EEPROM storage facility. Will be split from the Registry PR for easier reviewing.
  • File storage facility. Will be split from the Registry PR for easier reviewing.

Is the state of the document already well reflected?

Yes, the implementations of the modules follow the design described in the RDM.

It would be IMO helpful to introduce figure captions with numbering and refer to the figures from the text.

Will add

@tcschmidt
Copy link
Member

From the described architecture, implemented and working (though further testing is still needed):

* The **Registry**. I'm still adding tests and some missing documentation so I can remove the WIP status.

* **EEPROM storage facility**. Will be split from the Registry PR for easier reviewing.

* **File storage facility**. Will be split from the Registry PR for easier reviewing.

So let's concentrate on the implementation for the moment and let this memo rest in the meantime. If the solution stabilizes, we should see the RDM merge.

@danpetry
Copy link
Contributor

After reading the code a bit I found that the name "registry handler" wasn't that intuitive. "Storage facility" clearly indicates what it means, but since "registry" is the whole thing being implemented, "registry handler" is a bit too generic. Something like just "config group" would be clearer imo. What do you think?

@tcschmidt
Copy link
Member

@leandrolanzieri @jia200x are the formal issues of @danpetry addressed? Then I guess we should resolve the blocking comment.

@jcarrano
Copy link
Contributor

jcarrano commented Jun 7, 2019

@tcschmidt, We had some offline discussions with @leandrolanzieri, @jia200x that may lead to some changes (simplifications, I believe).

@tcschmidt
Copy link
Member

@tcschmidt, We had some offline discussions with @leandrolanzieri, @jia200x that may lead to some changes (simplifications, I believe).

Maybe summarize the outcome briefly here so that others can follow the state of development?

@jcarrano
Copy link
Contributor

jcarrano commented Jun 7, 2019

The general idea was to:

  • Make it less dynamic
  • Make it so that it can map easily to file-like structure.

The concrete changes are:

  • There is a (static) array listing configuration variables along with their type.
  • This makes a "listing" (export) method unnecessary. It can still exist, but it is not per-setting but rather a generic helper.
  • The "set" method takes an already parsed value (via a union) instead of a string.
    • The parsing code is generic.
  • It should be possible to create the config structures at compile time, in read-only memory.
  • Read-only data can also be presented via this interface.
  • The config system will look like a tree (but needs not be implemented as such)

Also:

  • It will be possible (by writing the appropriate glue code) to access config values via standard fopen(), read(). fwrite(). Think of /sys/ and /proc/. The important thing here is that if done properly, there is minimal overhead.
  • This is related to sensors/saul: it will be convenient to expose sensor meta-data and adjust their setting via the config interface.

@leandrolanzieri,

I just had another idea: what if we remove commit() method, instead having a "group/commit" config variable that causes a commit for "group" whenever any value is written to it. The main reason for doing this is that now the whole system fits under fopen()/read()/fwrite().

@jcarrano
Copy link
Contributor

jcarrano commented Jun 7, 2019

To expand a little more on why a table: by having a table of (variable_name, type, flags) that is handled by the generic config code we remove the need for each module to implement a long list of else if(strcmp(x, y)) .... The module can simply receive a number and parse it either via "switch... case" or using a function pointer table.

@stale
Copy link

stale bot commented Dec 9, 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 Dec 9, 2019
@leandrolanzieri leandrolanzieri added the State: don't stale State: Tell state-bot to ignore this issue label Dec 9, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Dec 9, 2019
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: RDM Area: Discussions on RIOT Developer Memos Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK State: don't stale State: Tell state-bot to ignore this issue TF: Config Marks issues and PRs related to the work of the Configuration Task Force
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants