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

Shared/Sink/Source: Provide getters/setter in addition to operator overloading. #67

Merged
merged 8 commits into from Jun 23, 2022

Conversation

aentinger
Copy link
Contributor

This fixes #66 .

@aentinger aentinger added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jun 9, 2022
@aentinger aentinger self-assigned this Jun 9, 2022
@sebromero
Copy link
Collaborator

@aentinger Sorry, I think I didn't provide enough context. I do think explaining that a shared variable works as a queue will help many users from the target audience to understand the concept. Therefore I'd add add() and poll() / peek() functions instead of get and set. Consequently in an example code I would be tempted to name a shared variable something like counterValues or similar to make clear that this is not an ordinary variable but a list.
IMO if the target audience was pure beginners the assignment operator alone might work, completely ignoring the fact that the values of a shared variable are buffered via queue. However, since we seem to target more advanced people I suggested to add the said functions.

@aentinger
Copy link
Contributor Author

Thank you @sebromero for providing this feedback. The main problem I see is that the target group is not clearly defined (and as I told you attempts to clarify it have been not successful). The way the APIs are designed right now (operator overloading) are tailored towards a beginner, who is bot bothered by the fact that a Shared variable is in fact a queue. One could argue though more advanced users who "get" the concept behind the Shared abstraction will not have a problem with the operator overloading either, as they understand how this works.

I'd like to invite @facchinm to share his thoughts on this subject, since he's been the originator of the operator overloading approach.

@sebromero
Copy link
Collaborator

I understand the idea. However I'd argue that from an educational point of view using an assignment operator for something that's not really an assignment is imo confusing for anyone who has the slightest idea about data structures. Ideally the users can leverage any prior programming knowledge.
The first thing I did was to look up the implementation to understand why I need to use an assignment for a non-assignment operation. I guess for the same reason most languages don't use the assignment operator to modify collections.
Also, if we rely on assignment and de-reference, how can we allow reading a value without removing it from the queue? E.g. if there are multiple readers?

That said, for complete beginners the assignment operator is a legit option but I have some doubts that they can dive straight into multitasking from the beginning.
It's obviously all subjective and a gut feeling as this is a completely new thing and we don't have any feedback or experience to back it up.

@facchinm
Copy link
Collaborator

The main idea was to easily "retrofit" existing projects by moving the variable definition to SharedVariables.h and leaving everything else as-is. I understand that, as things have been evolving, this doesn't make much sense, give that it must be explained thoughtfully. Maybe we should comment it out and just leave for future reference 🙂 ?

@aentinger
Copy link
Contributor Author

how can we allow reading a value without removing it from the queue?

For this there's already a peek() API available.

E.g. if there are multiple readers?

Such a problem (multiple consumers reading from a single producer) is indeed a problem that often traps beginners into the multi-threaded world. The Shared abstraction does not protect you from this which is mentioned in the documentation: here and here. On the flip side, Sink/Source semantics are solving this specific aspect of multi-threaded programming per design.

Moving on to questions of operator overloading ... I'm not feeling particularly strongly about this but one could argue that providing the operator overloading gives the code the appearance of a plain-old-data-type variable, i.e. if I'd to be assign a plain integer or a Shared integer goes not look any different:

int my_var;
my_var = 12;
Serial.println(my_var);
/* vs */
SHARED(my_var, int);
my_var = 12;
Serial.println(my_var);

This library will always be a simplification. If you want to leverage the full plate of what a RTOS has to offer you can always directly access the mbed-os API. For reference, consider how EdgeImpuls uses Arduino hardware. They basically start their own thread within setup() and its all mbed-os from there.

Moving I'd suggest to keep both operator overloading and getters/setters for now. But I would replace them with better names that what they have right now. Consequently I'm proposing push for adding and pop for retrieving data. What do you think? Have better suggestions? (I'm not a big fan of add because that's the name of every 2nd API out there, I might be convinced of poll but then again not really, as it does not imply the reading part of the API call as cleanly).

@aentinger
Copy link
Contributor Author

Alternate names that come to my mind are:

  • send/receive
  • write/read

although they sound rather generic.

What's your take @sebromero @facchinm ?

@aentinger
Copy link
Contributor Author

Decision per SIG Arduino Language meeting (and absent any further objections in this PR) is too add the getters/setters AND remove the operator overloading in a separate PR (so as to make it easily revertible).

@aentinger aentinger merged commit 3be602b into main Jun 23, 2022
@aentinger aentinger deleted the getters-n-setters branch June 23, 2022 13:24
aentinger added a commit that referenced this pull request Jun 23, 2022
No more assignment operator, type operator.

This leads to less confusion as discussed in #67.
aentinger added a commit that referenced this pull request Jun 28, 2022
No more assignment operator, type operator.

This leads to less confusion as discussed in #67.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] In addition to operator overloading also provide getters/setters
3 participants