search

purduesigbots / pros

Source code for PROS kernel: open source C/C++ development for the VEX V5 microcontroller
https://pros.cs.purdue.edu
Other
259 stars 76 forks source link

✨ FreeRtos queue wrapper class #691

Open Rocky14683 opened 2 months ago

Rocky14683 commented 2 months ago

Summary:

A wrapper class for freertos queue

Motivation:

Idk whether this should be in kernel, but this is very useful. For example, odometry and controller system.

Test Plan:

djava commented 2 months ago

Motivation? What usecase does std::queue not cover? I think the built-in vendor-provided queues are probably more designed to be used in cases where you don't have dynamic allocation, which does not apply for the V5.

Rocky14683 commented 2 months ago

Freertos queue receive will block current task until data got send into the queue.

Rocky14683 commented 2 months ago

It is mainly for the data sharing between 2(or multiple? Im not sure abot this) tasks.

ion098 commented 2 months ago

Freertos queue receive will block current task until data got send into the queue.

A thread safe queue class based on std::queue with this feature should also be possible, and would allow users to not worry about max queue size.

Also, it looks like the changes from your other PR #690 accidentally got added in this PR as well, you should probably fix that.

ion098 commented 2 months ago

Also this class won't work properly for types that have non trivial copy constructors (and I don't think there's any easy way of fixing that because of how the C API works).

Rocky14683 commented 2 months ago

I think that is not an issue because it's for passing data. It's not for passing complex objects. Also, the thread safe queue will work, but that shouldn't be in kernel.

djava commented 2 months ago

I think that is not an issue because it's for passing data. It's not for passing complex objects.

Strongly disagree, if we have a type template in C++ then it should work with C++ types. We can add a type constraint for trivially_constructible but at that point, wouldn't you rather just have something that actually works with everything? I don't see the point when it's strictly worse than another option.

Also, the thread safe queue will work, but that shouldn't be in kernel.

Then why is this? This is just a worse version of that.

Rocky14683 commented 2 months ago

Bcuz freertos queue is already in kernal(apix.h), I just wrap it with a class, it's easier to use.

djava commented 2 months ago

I just don't really think we should be encouraging using something that's not really worth using by making it easier? Who is this for? There's a reason its in apix.