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

✨ FreeRtos queue wrapper class #691

Draft
wants to merge 3 commits into
base: develop-pros-4
Choose a base branch
from
Draft

Conversation

Rocky14683
Copy link
Member

@Rocky14683 Rocky14683 commented Jul 24, 2024

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:

  • test 1->
截圖 2024-07-24 下午7 04 14
  • multiple queues
  • multiple tasks and queues interaction

@djava
Copy link
Contributor

djava commented Jul 26, 2024

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
Copy link
Member Author

Rocky14683 commented Jul 26, 2024

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

@Rocky14683
Copy link
Member Author

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

@ion098
Copy link
Contributor

ion098 commented Jul 26, 2024

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
Copy link
Contributor

ion098 commented Jul 26, 2024

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
Copy link
Member Author

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.

class Queue {
queue_t queue;

public:

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not going to use static assert, it does not work well with intellisense. I'll use concept.

Queue& operator=(Queue&&) = delete;

void delete_queue() {
pros::c::queue_delete(queue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pros::c::queue_delete(queue);
if (!deleted) {
pros::c::queue_delete(queue);
deleted = true;
}

}

~Queue() {
pros::c::queue_delete(queue);

This comment was marked as resolved.

@@ -820,6 +820,63 @@ struct Clock {
static time_point now();
};

template <class T>
class Queue {
queue_t queue;

This comment was marked as resolved.

@djava
Copy link
Contributor

djava commented Jul 27, 2024

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
Copy link
Member Author

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

@djava
Copy link
Contributor

djava commented Jul 27, 2024

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.

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 this pull request may close these issues.

3 participants