I didn’t realize that. I figured that based on Randy and Tridge’s comments, a reboot was required with those statements in the init() method. I would have to figure out where to put those statements if it is decided that a reboot is desired.
That’s what I thought too, based on the comments on GitHub. But I tested it and it re-initializes the servo when the params are changed. The code is more efficient with it in the init() function because it only calls it once.
I think we’re going to have to use a different appoach. As @tridge said, there’s numerous things that can be set with the ground station that can cause an unwanted hot-start. I dont’ think this is right for a high-end commercial-grade system, and I don’t think RC users should just have to “deal with it”. If you checked the oil in your car, slammed the hood shut and that caused it to start and go to full throttle, I think most people would consider that a real problem. I think it’s going to have to be flagged somehow and when any of these params that can be changed can cause a start, have a warning pop up in the ground stations that requires the user to verify that they understand, and require a second click to actually write the setting to the FC.
Once we figure out which direction we’re going with the code, whether we’re going to use the SRV_Channel library, write_rsc function, etc… I wrote one enhancement that adds two params and allows adjustment of the DDVP drive ramp and runup times separate from the main rotor.
I did this because some folks like a fast ramp like the hard-coded 2 sec/3sec was. I like the tail to ramp at the same speed as the main so it’s more like a mechanically driven tail. With compound heli’s I don’t know what would be desired. So I made it adjustable. This change can be merged eventually as see’s fit.
@tridge sorry I could not make the Dev Call this evening due to a prior engagement. We would like to arrive at a consensus on what to do to get a fix for DDVP drives into Copter master. We have it functioning and we’re flying the code built on 3.5.3. But we don’t know how to proceed on the PR to get the fix into master. The code I’m flying puts the call in the init() function. And it still hot-swaps the SERVO direction with the FC in unarmed state. I haven’t found a problem with how it works.
Requiring a reboot to make it take effect, I think at this point, is a separate issue. I think we’ll leave that alone for now since it’s been that way for awhile.
We could probably use a bit of help with it if the desire is to use the SRV_Channel library instead.
Open to further suggestions on how to proceed with a fix for the PR.
@ChrisOlson, @Rob_Lefebvre and @tridge, So I did some testing in the SITL for using the old write_rsc method and using the set_output_scaled SRV method. The old method used the min and max are taken from the SRV_channel and then a PWM is composed and written to the ESC. You can find the commit for the method using the set_output_scaled here
I tested several cases in all three modes using the output servo in norm and reverse. The only difference between the two methods that I see is that the set_output_scaled method allows you to change the min/max and rev parameters in both unarmed and armed modes whereas the old method only allowed them to be changed in the unarmed mode. I was also going to test AC3.3.3 to compare the results but I could not get the SITL to run properly with AC3.3.3. It bothers me that the user can still change the min and max in modes 2 and 3 where I think the user intentionally wants to output in terms of a range of 0 to 1000 based on the parameters for the set up of these two modes. It even says that the range is 0-1000 in the parameter descriptions. In these two modes I don’t see how changing min and max is desirable since it is clear that the mode setup is based on a range of 0-1000. That is primarily why I wanted to test 3.3.3 and understand how that works.
If the development team wants us to use the SRV_channel functions rather than composing the PWM in the rsc function, then for cases where the range is to be held at 0-1000, the min/max will have to be continually written to keep the user from changing it.
Thoughts?
Isn’t the settings from RSC_SETPOINT or the three-point throttle curve (Mode 3) scaled to the output range of the user-defined min/max?
It is scaled to the output servo min and max with the new method. And I guess for mode 3 with gas engines, the user needs to be able to set the end points and the reverse of the servo. I think we will have to adjust the parameter description for the RSC parameters that govern the mode 2 and 3. And review/update the wiki info to ensue it is accurate.
I don’t think we will, because the valid range is still 0-1000 for the RSC_SETPOINT, etc… It is just scaled to the output range of the servo defined by the min/max. Unless I’m missing something.
What is the perceived advantage using this method, vs defining the PWM in Heli RSC? With the output handed over to the SRV_Channel library what happens during radio failsafe, etc.? That is a special application case for heli.
So for RSC setpoint it does say that it is the value from 0-1000 that is oassed to the governor. Now the mode 3 parameters all say that output values are controlled by the rsc_min and rsc_max parameters which we removed. So that will need to be modified and may want to add in the RSC setpoint parameter description that the min and max servo values need to be 1000 and 2000 to get correct output.
I guess the perceived advantage is not managing the min/max and rev parameters and reduced code in the RSC write method. I will have to test the failsafe. My intuition is that this would not affect it because we are still calling on the SRV library for the scaled output as was done for the pwm output.
Ah, Ok. I see now what you mean.
Correct. I should test this too in a real heli. Need to make sure both tail and main stay lit with loss of radio signal.
I will test it in the SITL too. If it passes then I will add it to the PR and hopefully we can get this into master.
Test it in the SITL and if it passes there go ahead and make the PR change. I want to put this thru the full battery of tests, including autorotations and radio failsafe, plus limiting the min/max and trying the reverse function to see what effect it has, in the real thing to make sure it functions like the code I’m flying now. It will likely be late tomorrow afternoon or Saturday before I get to flight test it and I’ll let you know if I find anything wrong with it.
I wanted to verify that this is the same in the stock code, the ArduHeli code (which is different from the PR), and the newest code that is potentially for the PR.
This is copied and pasted from the current Complete Parameter List on the wiki:
H_RSC_PWM_MIN: RSC PWM output miniumum
This sets the PWM output on RSC channel for maximum rotor speed
Range
0 - 2000
H_RSC_PWM_MAX: RSC PWM output maxiumum
This sets the PWM output on RSC channel for miniumum rotor speed
Range
0 - 2000
It is the same behavior no matter what code is being used. So using the ArduHeli dev code as an example (this code places the call in the init() function and has adjustable parameters for DDVP ramp/runup, composes the PWM output in Heli RSC). The H_RSC_PWM_ params are gone from this code so SERVO8_MAX was adjusted as a test.
The first runup was done with min/max set to 1000/2000. The second runup was done with min = 1000, max = 1800. It scales the output to the servo accordingly (although the wiki is wrong on which direction it does it unless the servo is reversed):
So using the SRV_Channel library for output doesn’t change anything from that respect, and I was almost certain it didn’t. But had to test it to make sure. This was tested with MODE = 2, so this description for this parameter is not even correct in the wiki because it only applies if the min/max are not messed with and set to be 1000/2000.
H_RSC_SETPOINT: External Motor Governor Setpoint
PWM in microseconds passed to the external motor governor when external governor is enabled
Range Increment Units
0 - 1000 10 PWM in microseconds
Since MODE = 3 does not work very well with a piston engine (piston engines do not have a three-point linear torque curve), the more I look at the RSC system, the more convinced I become that it needs a complete overhaul. Why?
- 90+% of our users are using MODE 1 for throttle control because:
- a) the radio provides the most flexibility in setting up throttle/pitch curves for either electric or ICE
- b) this mode works with governors with the flexibility of being able to change throttle setpoint from the radio instead of having to adjust params
-
MODE 2 is designed for electric governors but it can be broken by adjusting the min/max/rev params
-
MODE 3 does not work very well for ICE. I use it in one electric heli with no governor and it works OK there since electric motors have a much flatter torque curve than ICE. But it is still a mode that very few people use, just because MODE 1 is better.
It becomes apparent (to me) that what is needed is
- MODE 1 providing pass-thru as it is now
- MODE 2 for electric governors, but the min/max/rev is not adjustable
- MODE 3 for ICE governors where it’s the same as MODE 2 except reversing the servo and setting min/max may be needed. But drop the three-point throttle curve because it’s a bad design - everybody I know of will use MODE 1 if they have no governor
- MODE 4 with internal governor function in the Pixhawk that reads rpm input from an external sensor (on an Aux pin) and adjusts the output to the servo accordingly (I’m working on this mode)
This is all outside the scope of this PR and fix for DDVP. It just became evident this whole system is less than ideal so I thought I’d voice my opinion as to why.
I actually have internal governor code working on a gasser. It’s not 100%, but it’s a start. It was based on 3.3 and is probably not portable forward to 3.5, but the concepts are there.