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
actuators[ ] array of structs with pprz units and driver units #3205
base: master
Are you sure you want to change the base?
Conversation
The issue with this (actuators in pprz_t) is that the actuators message is now also in pprz_t. So if you need to read the actuators position to adjust the trim (which should be in "driver" unit), you will have a good chance to make errors doing the conversion or just forgetting to convert it to the correct scale. |
Agreed |
3536282
to
f461e8c
Compare
Changed to actuator struct. If you have naming recommendations let me know. This should allow us to simulate actuators directly now in I didn't change it yet because the actuator indexes often do not match and requires some harmonization (see #3203) |
f461e8c
to
e495727
Compare
… when sending to the actuator_driver. actuators becomes an array
e495727
to
6fc2c2c
Compare
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.
Great development!
@@ -72,7 +79,7 @@ extern int16_t actuators_pprz[ACTUATORS_NB]; | |||
* @param _n actuators name as given in airframe file, servos section | |||
* @param _v new actuator's value | |||
*/ | |||
#define _ActuatorSet(_n, _v) Set_ ## _n ## _Servo(_v) | |||
#define _ActuatorSet(_n, _v) Set_ ## _n ## _Servo(_v,_v) |
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.
Here you pass the value _v
twice, but they should be pprz and driver values. It seems that something is wrong. We either do the mapping pprz to driver should be done in Set_XXX_Servo
functions (and they take pprz inputs), or we store the pprz_val
before calling this function (and they take driver inputs as it is currently the case).
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.
This macro is weird indeed and points to an inconsistency in the project: it is used in code that sets actuator values directly: e.g. switch servo sets the actuator to 1200 us. The correct way would be to put it to MAX_PPRZ. I can harmonize that.
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'm in favor of harmonizing the servo setter with PPRZ values. It would make the code actuator independent.
struct actuator_t { | ||
pprz_t pprz_val; ///< Actuator value in PPRZ units | ||
int16_t driver_val; ///< Actuator value in driver units (scaling from servo in airframe.h) | ||
}; |
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.
This is nice to have an actuator structure, and it could / should hold even more information, like the type of actuators, the status, its position or speed if measured, etc. Should we start already with this (and maybe change it later again), or already try to add future elements, even if not used yet ?
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.
It is very easy to add things later. I'd rather only add variables while that are used.
In this PR, as a first step, my goal is to be able to run the simulator directly from actuators everywhere (both INDI and rotorcraft)
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.
But I agree the rpm/position feedback could move to this struct a.s.a.p.
Anybody able to help?
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 was planing to do this after finishing the reorganization of stabilization, if I can finish that someday...
<dl_setting VAR="actuators[5]" MIN="900" STEP="1" MAX="2100" shortname="chan5"/> | ||
<dl_setting VAR="actuators[6]" MIN="900" STEP="1" MAX="2100" shortname="chan6"/> | ||
<dl_setting VAR="actuators[7]" MIN="900" STEP="1" MAX="2100" shortname="chan7"/> | ||
<dl_setting VAR="actuators[0].driver_val" MIN="900" STEP="1" MAX="2100" module="modules/actuators/actuators" shortname="chan0"/> |
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.
It would be nice to have a getter function or macro for that, in case the structure is modified in the future
In simulation / NPS, we would like to simulate all actuators. The
actuators[ ]
array can not be used because it has different units: e.g. PWM 1100 - 1900.With the new system of sending actuators to different actuator drivers, the conversion to actuator_driver units is best done when sending to the actuator_driver.
actuators_pprz
is not needed.