ESC Telemetry plugin: add diagnostics support #1705
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vooon
reviewed
Feb 16, 2022
There was a problem hiding this comment.
I would rather having separate diagnostics for each ESC than each value.
Config part looks ugly. You can use private node handle to shorten paths.
auto pnh = ros::NodeHandle(ros::NodeHandle("~esc_telemetry"), "diagnostics")
pnh.param("enabled", diag_enabled, false);
...
|
Also you can move limit init to custom diag class. |
I will work on that than.
Will that not create a bunch independent of limits per ESC? That is a bit too much "configurability"! |
Not necessary to use different names. E.g. |
|
I did some of the changes you requested, but I think I did not use it the way you planned. Can you take a look at it? |
vooon
reviewed
Feb 16, 2022
| pnh.param("curr_max/nok", this->_curr_max_nok, 10.0f); | ||
| pnh.param("curr_max/ok", this->_curr_max_ok, 8.0f); | ||
| pnh.param("rpm_max/nok", this->_rpm_max_nok, 12000); | ||
| pnh.param("rpm_max/ok", this->_rpm_max_ok, 9000); |
There was a problem hiding this comment.
Perhaps i'd generate that list by cog.
// [[[cog:
// fields = [
// ("temp", (1.0, 0.0), (85.0, 90.0), ),
// ...
// ]
//
// for field, min_, max_ = range fields:
// for fm, lim = zip(("min", "max"), (min_, max_)):
// for fn, l = zip(("ok", "nok"), lim):
// cog.outl(f"""pnh.param("{field}_{fm}/{fn}", this->{field}_{fm}_{fn}, {l});""")
// ]]]
There was a problem hiding this comment.
done, now I just need to find out how to run the cog compiler
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, sorry, please use for _ in fields. = range is a golangism.
There was a problem hiding this comment.
Fixed it, but now it has other issues :(
amilcarlucas
force-pushed
the
esc_telemetry_diagnostics
branch
3 times, most recently
from
c39664b
to
6f2276c
Compare
vooon
reviewed
Feb 17, 2022
| int rpm_min_nok; | ||
| int rpm_min_ok; | ||
| int rpm_max_nok; | ||
| int rpm_max_ok; |
There was a problem hiding this comment.
We can generate most of that fields by cog also. Will need to move fields list and extend it to have type.
| lim(lim_) | ||
| {} | ||
|
|
||
| const Limits& lim; |
There was a problem hiding this comment.
Ah no, we can't really use const ref on temporary object. https://stackoverflow.com/questions/15513734/const-reference-as-class-member
Try to add limits object to the plugin class and pass it to diags.
amilcarlucas
force-pushed
the
esc_telemetry_diagnostics
branch
3 times, most recently
from
9841fa9
to
7c45aef
Compare
|
@amilcarlucas could you please post gcc error here? |
|
It is deep and ugly: Thanks for your review and help |
|
Add |
|
Thanks! It compiles now. But cog does not: |
amilcarlucas
force-pushed
the
esc_telemetry_diagnostics
branch
from
9dd8487
to
613f38d
Compare
|
What version of python do you use? For f-strings you need >= 3.6. |
amilcarlucas
force-pushed
the
esc_telemetry_diagnostics
branch
from
613f38d
to
f56d42c
Compare
|
My bad I was using I will move the diagnostics to the dynamically created ESC objects ... soonish |
|
|
amilcarlucas
force-pushed
the
esc_telemetry_diagnostics
branch
from
f56d42c
to
14fc11d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This create separated diagnostics for:
Should I instead create one diagnostic per ESC?