Hi all,
I’m diving into ardupilot development (again, I’ve had an on/off relationship with the system for a few years) and in the process of developing some libraries I’ve run into some questions with regards to the current RPM implementation.
ap_rpm._maximum, ap_rpm._minimum and ap_rpm._quality_min are declared and used as arrays despite the parameter configuration and reference manual implying they should instead be treated as universal parameters. The parameters however only allow you to set the first set and the reference manual indicates that these parameters would apply to both.
ap_rpm._quality_min only every has its 0’th element addressed so it’s already treated as a non-array variable anyway.
As I understand it ap_rpm._maximum and ap_rpm._minimum are addressed by their indexes despite not being initialised to any particular value, currently this is caught in the below extract and the result is no limits are applied.
float maximum = ap_rpm._maximum[state.instance];
float minimum = ap_rpm._minimum[state.instance];
...
if ((maximum <= 0 || rpm <= maximum) && (rpm >= minimum)) {
I guess the questions I have are:
- Is this the correct interpretation of the current function of the library?
- Is this done intentionally, and is there any particular reason RPM is limited to 2 inputs only with common limits?
Expanding AP_RPM to allow A) More inputs and B) Different limits for each input would be quite desirable my end of things and doesn’t seem that much of a chore for me to change, I’m just looking at the current restrictions and wondering if there was some reason these limitations were enforced at some point? If not I’m happy to clean that up and expand to say 6 inputs?
RPM reference:
https://ardupilot.org/copter/docs/common-rpm.html
github pull-request are welcome, and a lot better to discuss code changes than forum posts
I¢m diving into ardupilot development (again, I¢ve had an on/off relationship with the system for a few years) and in the process of developing some
libraries I¢ve run into some questions with regards to the current RPM implementation.
ap_rpm._maximum, ap_rpm._minimum and ap_rpm._quality_min are declared and used as arrays despite the parameter configuration and reference manual implying
they should instead be treated as universal parameters. The parameters however only allow you to set the first set and the reference manual indicates that
these parameters would apply to both.
ap_rpm._quality_min only every has its 0¢th element addressed so it¢s already treated as a non-array variable anyway.
As I understand it ap_rpm._maximum and ap_rpm._minimum are addressed by their indexes despite not being initialised to any particular value, currently
this is caught in the below extract and the result is no limits are applied.
float maximum = ap_rpm._maximum[state.instance];
float minimum = ap_rpm._minimum[state.instance];
…
if ((maximum <= 0 || rpm <= maximum) && (rpm >= minimum)) {
I guess the questions I have are:
- Is this the correct interpretation of the current function of the library?
Looks lik eit.
- Is this done intentionally, and is there any particular reason RPM is limited to 2 inputs only with common limits?
There probably was, but it wasn’t captured in comments, so we may need
to re-learn it that reason :-/
You definitely seem to have found a bug. If we didn’t want to add more
parameters (quality/min/max) for the second pin, we should stop using an
array to store min/max/quality which will stop future indexing bugs.
Expanding AP_RPM to allow A) More inputs and B) Different limits for each input would be quite desirable my end of things and doesn¢t seem that much of a
chore for me to change, I¢m just looking at the current restrictions and wondering if there was some reason these limitations were enforced at some point?
If not I¢m happy to clean that up and expand to say 6 inputs?
Improvements are always welcome, but we do have to be very careful to not
break existing setups; for example, if we were to have different max/min
parameters for each of the inputs then existing users may end up with a
broken system when they upgrade.
@peterbarker Thanks for your insights, basically confirms what I suspected.
With regards to breaking existing setups I believe in this particular instance we would be fine to expand those changes without repercussions. Any implementations that didn’t use the second instance won’t be affected anyway and those that are currently apply no limitations in practice anyway.
We could either expand the array implementation and initialise those new parameters at their min/max respectively and cause no change to the current functionality, or reduce them down to a single variable and the code would begin to function as per the documentation.
It’s a bit of a backwards situation but in this case I would suggest the safest course of action would be to expand the arrays as nothing is going to go breaking in the background. As an added bonus it expands the capabilities of the library slightly for anyone who might be chasing that capability.
With regards to expanding to allow for more inputs well I’ll have to dig into everything a bit further there before I do anything. I was mostly testing the waters, I don’t want to go changing something that may have been very deliberately capped at 2 inputs. My suspicions are that the RPM implementation was made for the gasser and heli guys who rely on the feedback for the functionality of their aircraft and accordingly a twin is about all it would ever be used for. In my particular use case I’m chasing the feed off a set of Hobbywing XROTOR6s, primarily for vibration analysis (+ other diagnostics) and so I’m needing the extra inputs.
@amilcarlucas More than happy to do so once I work out what the appropriate course of action is. The forum post wasn’t so much for the code change as it was the reasoning behind the current implementation as it stands and what the process is behind proposing and implementing any changes. I was under the impression that was the idea behind the forum, vs a github pull for bug patches?
When it comes to a change like this proposal in the future is it best to go straight to the github? If so, when do I use the forum?
Thanks guys for your responses, I’ll set myself a little task to patch that bug atleast and perhaps expand the inputs if appropriate, it’ll be a good little first exercise for me.