sys/saul_reg: support for saul context parameter#9099
sys/saul_reg: support for saul context parameter#9099ZetaR60 wants to merge 7 commits intoRIOT-OS:masterfrom
Conversation
bergzand
added
Discussion: RFC
bergzand
left a comment
bergzand
left a comment
There was a problem hiding this comment.
Some pre-coffee initial remarks
sys/include/saul_reg.h
Outdated
| */ | ||
| typedef struct saul_complex_ptr { | ||
| struct saul_reg *real; /**< pointer to real device in SAUL registry */ | ||
| uint8_t imag; /**< pointer to imaginary device duplicate */ |
There was a problem hiding this comment.
This is more an index than a pointer right?
There was a problem hiding this comment.
It is an analogy. It is a "pointer" that points to an "imaginary" device by way of an additional parameter. So far as the SAUL functions are concerned: It is as if there is an imaginary part of memory that holds the (actually nonexistent) data about the device.
There was a problem hiding this comment.
Ah, IMHO using pointer in the comment is a bit confusing. Thanks for clarifying!
sys/include/saul_reg.h
Outdated
| * @return .real = NULL if no device is registered at that position | ||
| */ | ||
| saul_reg_t *saul_reg_find_nth(int pos); | ||
| saul_complex_ptr_t saul_reg_find_nth(int pos); |
There was a problem hiding this comment.
I would change this to
int saul_complex_ptr_t saul_reg_find_nth(saul_complex_ptr_t *node, int pos);
and have it return 0 or 1 based on if there is a device at the position. This way checks if the device at @p pos exists are still easy and you have only an extra pointer in the arguments.
There was a problem hiding this comment.
saul_reg_find_nth(pos).real != NULL gives you an easy way to test this as well. If you want to assign and test at the same time you could even do (foo = saul_reg_find_nth(pos)).real != NULL. Both are similar to the previous behavior (with just the addition of .real).
There was a problem hiding this comment.
Problem is that this will require a double pointer to be passed in the argument, because you want saul_reg_find_nth to be able to modify it rather than its own local copy.
sys/include/saul_reg.h
Outdated
| * @return .real = NULL if no device of that type could be found | ||
| */ | ||
| saul_reg_t *saul_reg_find_type(uint8_t type); | ||
| saul_complex_ptr_t saul_reg_find_type(uint8_t type); |
sys/include/saul_reg.h
Outdated
| * @return .real = NULL if no device with that name could be found | ||
| */ | ||
| saul_reg_t *saul_reg_find_name(const char *name); | ||
| saul_complex_ptr_t saul_reg_find_name(const char *name); |
ZetaR60
force-pushed
the
RIOT_saul_context
branch
from
1a33070 to
617cce3
Compare
ZetaR60
changed the title
|
Now tested and working on mega-xplained. |
ZetaR60
force-pushed
the
RIOT_saul_context
branch
from
bfdf5b3 to
2b18600
Compare
|
Rebased to pick up any new drivers, and API change support for everything in drivers/ added. |
ZetaR60
added
the
CI: ready for build
ZetaR60
force-pushed
the
RIOT_saul_context
branch
from
b2d140e to
a3cdd71
Compare
ZetaR60
force-pushed
the
RIOT_saul_context
branch
from
a3cdd71 to
e36b308
Compare
|
Rebased on trunk and resolved conflicts. |
|
By just (very quickly) looking over this PR I have to say I dont like it. I moves SAUL into a direction it was never designed for, but I need more time to look into this PR better... |
|
@haukepetersen Thanks for taking a look. Unfortunately there doesn't seem to be a great way of using SAUL for many similar devices (i.e. a driver board with many drivers, or an analog input board with many multiplexed inputs). I have tried to move the context parameter into the background as much as possible, by treating each dev+context as a separate virtual device. Once you get a chance to examine it in more detail, we could discuss a redesign to try to improve things. |
|
@ZetaR60 no problem and I have to excuse in advance that I will be mostly offline for about a month starting next week... My main problem with this PR is, that it is based on a wrong understanding of what SAUL actually is and what SAUL is supposed to do, leading to this PR further introducing code that brakes the concept of SAUL. The main idea of SAUL is to provide unified access to device drivers, based on their perception as actual physical entities of some sort, so interacting with devices on a high-level based on what they do/represent in a functional scope (hence SAUL is based on physical units tied to actual 'physical' device types). It was never meant as an abstraction for logical, generic MCU and device peripherals (such as GPIO/ADC). This is the domain of the periph interface. Now in the past there was some effort and approaches to directly map things like GPIO pins and ADC lines to the SAUL interface, but this was typically done in scope of tying these peripherals to actual physical 'devices', such as a light (LED), or a high-level button/switch, tying the actual peripheral instance used with some additional information (i.e. pin polarity). The cases where peripherals are 'blindly' thrown into the SAUL registry, without tying them to any specific, high-level entity are already very borderline... If I am not mistaken I expressed my concerns about this multiple times in the past. Now let me try to sketch what I think should be the way forward to handle cases introduces e.g. with drivers like #9054 or the So in short:
Sorry for the long text, I hope my thoughts are clear enough to be followed :-) |
|
@haukepetersen Thanks for the comprehensive explanation! I feel that in the general sense of what the context parameter can do it does break with what you have explained as the goal of SAUL. However, my intention here and the specific use I have put it towards I think is a much smaller conceptual extension to SAUL. The goal of this PR is to decouple memory usage in the SAUL registry from devices that appear in the SAUL registry. The context parameter when used creates the appearance of more devices in the SAUL registry without actually having separate entries in memory. Each of these virtual devices is intended to be a physical entity (such as a pin). As far as users of the SAUL interface are concerned, the only difference between a virtual device and a real one is that multiple virtual devices happen to share settings with adjacent registry entries, that's all. I think that it may be possible to better follow what you have laid out as the goals for SAUL by making the context parameter more invisible to users of the SAUL API. My concern with a separate API for these sorts of devices (arrays of physical entities that share settings), is that it will be replicating work already done for SAUL, and that you will end up with an API that is almost the same as SAUL but with a context parameter (or similar). Unfortunately, without a parameter similar to the context parameter here, work from SAUL cannot be directly reused for this new API. I suggest that the addition of the context parameter to SAUL is the most straightforward and efficient way to accomplish handling of these devices. I hope that we can salvage work from this PR, and simply adapt it better to your vision of the goals of SAUL. I think I should take a moment to outline my ultimate goal with this work: I am working towards CoAP access to all sensors and actuators. SAUL is in the perfect position to do this, and it supplies much of the detail required for CoAP. The primary limitation I have seen so far is the memory usage inefficiency when handling arrays of devices that share settings. |
|
I think the problem you have is more with I see your problem of I will explain my point of view below: I will use the word "collection" for a structure with a group of objects, list, dict, tree (meaning in Python) Regarding the API change, I see this change as integrating "collections" (list/dict) to the saul device and I see it as very specific need. For me:
A python equivalent By adding this context pointer to Which could make sense for an object represented as a group. However, if I have a sensor with only one entity, I need to do an additional selector: Here I should select it with As I see it, And if saul store leds as a list, I am bit screwed, I need to internally store Having a simpler access to For the argument of saving memory: On the version of master before this PR: With this PR: Same RAM but more ROM usage. For saving memory, you can just put some part of your |
|
@cladmi Thanks for the analysis. Let me clarify the PRs goals a bit in the context of what you said: The LEDs and buttons on mega-xplained are just a toy problem to demonstrate how the context parameter works (I do not expect it to be a valid demonstration of memory efficiency); the real goal is to be able to efficiently handle automation tasks requiring dozens of almost identical devices in SAUL (such as generic drivers, or multiplexed analog inputs). The way I was attempting to do efficient handling of identical interfaces in this PR is to separate the abstract idea of an object in SAUL from the actual representation in memory. You can see this in how I have modified I am hoping that by improving the use of the context parameter until it is invisible to users of the API, it satisfies the goal of efficient access of identical interfaces while being only a very limited extension of @haukepetersen 's vision for SAUL. Improvements to the use of the context parameter would only require modifications to this PR, rather than throwing it out completely. In short: to use your Python example: The goal is for this to work in exactly the same way for users of the API. The change would be that the |
|
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
bot
added
the
State: stale
4 participants
Note: some of this initial description is now obsolete. The complex / imaginary analogy was dropped, since it was confusing. The method of passing the context parameter is now a conventional argument (which is simpler, but requires more changes).
Description:
Currently SAUL does not handle large arrays of nearly identical devices well. For example, if you want to write to a large number of GPIO pins, or read from a lot of analog inputs, you must currently have a struct with separate parameters for each of them. It would be nice to be able to tell SAUL that you have an array of devices, and to share all parameters between them except for an index value.
My idea on how to do this:
How the duplicates would be used:
saul_entries[i].imag_countin auto_init_foo.dev.imag != 0the value of imag is stuffed into the first byte of the device descriptor. The first element of the device descriptor struct MUST be a byte intended for this use. C guarantees that the address of the first element is the same as the address of the struct. This value could also be passed by changing the definitions of saul_read_t and saul_write_t, but that would require a much more extensive rewrite.This is intended to be as conservative a change as possible, with almost all new behavior confined to when
imag_count > 0.Currently changes in this PR are limited to an example, because I want to wait for some feedback on the method before doing more extensive edits.
See also: