Skip to content

Shared Variables queue issue #2

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

Closed
pnndra opened this issue Sep 5, 2020 · 2 comments · Fixed by #3
Closed

Shared Variables queue issue #2

pnndra opened this issue Sep 5, 2020 · 2 comments · Fixed by #3

Comments

@pnndra
Copy link
Contributor

pnndra commented Sep 5, 2020

it seems to me that the line below is wrong:
https://github.com/bcmi-labs/ArduinoThreads/blob/9f652184c5bbc5356e619b5fbc4e4791ea208de0/Arduino_Threads.h#L17

basically it's putting a reference to the current object in the queue but since the object is not copied, in case two elements are put in the queue these will contain the same pointer and when reading the value both items will return the same as they're reading the member of the object, which in the meantime may even have changed again.
https://github.com/bcmi-labs/ArduinoThreads/blob/9f652184c5bbc5356e619b5fbc4e4791ea208de0/Arduino_Threads.h#L9-L13

if we want to really have a queue of more than one item we need to allocate memory for that and push/pull values from there.

bottom line: do we really need more than one item in the queue?

@aentinger
Copy link
Contributor

aentinger commented Sep 7, 2020

Hi @pnndra 👋

As I understand it a copy to the current Shared<T> instance is added to the queue in this line. The value might be handed over by reference but the type of the queue is Shared<T> leading to a call to the (copy)-constructor at this point. (Since internally rtos::Queue<T, N>::put(...) copies the value into it's own queue-assigned memory region the external shenanigans don't matter much).

However, I've got myself other issues with the implementation. Why is a new instance of rtos::Queue<Shared<T>, 16> allocated on the heap everytime an instance of Shared is created? I think the following code should work just as well:

template<class T, size_t N>
class Shared : private rtos::Queue<T, N> /* Private inheritance - one queue / class but don't leave it's API user accessible */
{
  public:
    operator T() const {
      osEvent evt = get();
      if (evt.status == osEventMessage) {
        T * val = reinterpret_cast<T*>(evt.value.p);
        return (*x);
      }
    }
    T& operator= (const T& val) {
      put(&val); /* rtos::Queue<T, N>::put(...) performs a copy of val into OS internal memory. */
    }
};
/* ... */
Shared<float, 5> temperature;

@pnndra
Copy link
Contributor Author

pnndra commented Sep 11, 2020

hi @aentinger your interpretation seems wrong to me. actually queue.put does not seem to copy the object but rather just stores the pointer to the object being passed. as you can see in the example here: https://os.mbed.com/docs/mbed-os/v6.2/apis/queue.html they allocate and free the objects which would not be needed if they copied it in the queue. this also explains why each time you pass the Shared it's not really allocating a new queue...
if you look at the code for queue (https://os.mbed.com/docs/mbed-os/v6.2/mbed-os-api-doxy/_queue_8h_source.html) and the prototype of the underlying code (https://www.keil.com/pack/doc/CMSIS/RTOS2/html/group__CMSIS__RTOS__Message.html) you can see that only a pointer is passed and at some point its type is lost so there's no way i see the underlying code could actually make a copy of the object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants