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
Feature/carla carma state integration #49
Feature/carla carma state integration #49
Changes from 9 commits
c8b2319
ac4297f
f48fb0a
069df9d
34a1dab
1fc0220
5b1f1f3
8a7247d
fa2a439
55cbd87
cb324e1
be2c7bc
4b81b86
9b06ce9
9a8daba
581a5f2
912bc63
366b72c
9f35bfe
bd4747a
81b8c89
27e0c5f
248ed7c
d2c943e
c005af8
979a75c
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 think this function is equivalent to
name in selected_plugins
if you want to simplify things a bit.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.
Is the intent to do something with this plugin object? It looks like it gets created here in the callback, some data gets copied into it from the callback data, and then it falls out of scope and is deleted. If the intent is to save the data for later this probably needs to get added to a list somewhere with a longer lifetime (maybe the global plugin_list?). Or if it's just for temporary use, I think we can do away with it entirely and just use the data directly from the callback.
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 think we probably want to just call this service once per plugin. We'll be getting these plugin status updates at ~10hz I think, and while I don't think it'll necessarily cause issues to call the service repeatedly with the same data, it's not how the service is intended to be used. It's probably best to call it maybe only the first time we see a given plugin report that it's registered, or maybe even just directly after our initial call to the get_registered_plugins service.
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.
Can you explain a little bit of the intent here? We pass in a plugin object (containing all the plugin data fields) and if that exact object exists already in the plugin list, we get that object back, otherwise we get None? Is the intent for this to check if we've seen data about this plugin already?
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 think the types on line 23-25 should be looked at, this are pretty abnormal python types so I'm not sure where they're coming into play here. Windows API stuff and the XML packages definitely aren't needed for this, and I'm not sure what the "typing" package is.
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.
Doesn't look like this state variable is used in this function.
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'd recommend moving these constants out of this function to be global constants instead.
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.
Is this errorDescription used anywhere? Not sure what all this error handling is intended for, it can probably be simplified to a check for the "NO_ERROR" value and a generic exception or error notice on all other cases. I don't think there's a meaningful way for this node to handle any of these other errors outside of maybe waiting a brief delay and retrying right?
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.
Any update on this?
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'd move this subscriber setup to something like the initialize method, just so that it's setup somewhere that's more likely to only be called once.
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.
Any progress on this todo? I think maybe the
/clock
topic might be a decent place to start but I'd double check that with @chengyuan0124There 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.
This is going to infinite loop the node right? If there's no routes (for example, maybe they haven't finished loading yet) then we'll hit this line and never get to check for any routes again. I'd recommend re-working this logic.
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 think for this we'd probably be better of throwing an exception or logging an error (or publishing system FATAL?) since the user's intent (run this route) can't be executed.