Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add controller manager services #139
Add controller manager services #139
Changes from 16 commits
5deeaf6
f00ecec
91e1767
bef08e2
27206ea
47b170f
81a8d21
160a7c7
4c6a609
2b81d5d
e4e226c
64ff2b2
3481676
62df3aa
bc63827
3650c38
9c05a5e
0967122
2805044
ef1187f
0067623
b6ea360
2fba683
dd02801
ae10741
9aff325
528d1c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this naming kind of confusing. If I understand this correctly, these indices are used for indicating which of the two lists within the double buffered lists are active, is that right? That also would imply that they must not be pointing to the same index. What about having only one variable (maybe call it
active
or similar) and use the!index_
to update the currently inactive one?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they can be pointing to the same index. From the documentation above:
What is missing in that documentation is an explicit mention that these "states" can overlap. I will add this
You can have an:
update()
)update()
)update()
but starting/stopping a controller on another thread)update()
)That's why we need two variables to track this. Yes, they could be two booleans, but right now at least there's one usage for the
-1
initial value on the rt list, that I guess can probably be reworked.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, don't want to make this tedious, but I still don't quite get this.
What's the purpose of "update + unused_by_rt" and "outdated + unused_by_rt". I understand that there's a phase where the controller list is getting updated while not being used, but that shouldn't influence the current state of the
update()
IMO.Why can't you just use a pointer or iterator to the currently used list in the
update
function, update the "unused" list and when that list is fully updated atomically swap the iterators?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can use two iterators instead of two ints, but it's always two variables to indicate:
I may be missing something, but I believe we always need this "acknowledge" from the RT thread, because the non-RT thread is waiting for it to use the new list and cleanup the old one, and that requires two variables, or at least a more expressive variable.
But again, coming to my point for not rewriting this from scratch... This works, and has worked for the last decade, and is efficient. We have a lot of pending work on other fronts. Yes it's ugly and there's probably better solutions to it, but I feel like we're investing a lot of time for some code that will be forgotten like the old version was, because it works.
At least I suggest we move it to an issue and look at it again after this PR is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok