Skip to content
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

Ap_Frsky_Telem: added preliminary support for sending RPM down the passtrough link #3

Merged
merged 2 commits into from Dec 20, 2019

Conversation

yaapu
Copy link
Contributor

@yaapu yaapu commented Dec 8, 2019

Copter: added new constructor to pass a pointer for the rpm sensor library
Frsky: added encoding of the rpm info as 16bit unsigned int and updated the scheduler to support packet type 0x50A0

if (_rpm_sensor == nullptr) {
packet_ready = false;
} else {
packet_ready = (_rpm_sensor->enabled(0) || _rpm_sensor->enabled(1));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. We'll be able to send the rpm on the passthru if either sensor is enabled. Does this still allow for use of external sensors for engine(s)? In heli we use it for rotor rpm for governor, and runup_complete() (HeliPilot only - this never got into ArduPilot). And the plan is to add a critical headspeed warning alarm (MavLink message) to get the pilot's attention in event of engine or drivetrain failure in Auto flight mode. Having this in the passthru makes it a viable reality since the latency to a GCS is too much to make it usable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still allow for use of external sensors for engine(s)?
external as wired on the s.port bus than yes, those do not interfere with passthrough.

@MidwestAire
Copy link
Owner

MidwestAire commented Dec 8, 2019

@yaapu in HeliPilot I use the rpm to update the runup_complete flag using an actual measured headspeed value and comparing it against the critical rotor speed
https://github.com/ChristopherOlson/HeliPilot/blob/de3856aa94787d8345d7b333b8d95df61f5a893a/libraries/AP_Motors/AP_MotorsHeli_Throttle.cpp#L170

I had actually PR'd this to ArduPilot some time back. But not all helicopters have a speed sensor. In HeliPilot we do not approve flight without one for combustion engine heli's. Having the tachometer for the rotor on the RC telemetry, displaying exactly what the Throttle library (also doesn't exist in ArduPilot) is "seeing" for headspeed will be an invaluable tool for the pilot.

@yaapu
Copy link
Contributor Author

yaapu commented Dec 9, 2019

with this implementation the radio would know the headspeed rpm but would have no way to trigger alarms based on it, do you plan on adding them via OpenTX?

@MidwestAire
Copy link
Owner

Via MavLink message, a critical warning similar to EKF or compass variances.

@MidwestAire
Copy link
Owner

@yaapu at least at the moment that is my idea on how to do it. Many times in Auto flight mode the helicopter is a long ways downrange and if a power or drivetrain failure occurs there is limited time for the pilot to do something about it before the altitude controller stalls the main rotor attempting to hold altitude. If the pilot can catch it early enough then autorotation is still possible.

Having such a warning on the OpenTx side would mean using logic switches to prevent triggering the alarm when the helicopter is on the ground, or running up. On the other hand, the autopilot knows the status of the helicopter. It can do a simple evaluation of, we are in dynamic flight (a special heli_flags), and rotor_rpm is above critical (already build into the runup_complete flag). If we have dynamic_flight = true, runup_complete = true and rotor_speed < critical_speed we need to let the pilot know about it so he/she can get out of the altitude controlled flight mode before the rotor stalls. This prevents triggering the warning unnecessarily.

@yaapu
Copy link
Contributor Author

yaapu commented Dec 9, 2019

ok, so you plan on adding one extra bit to the ap_status bitmask?

@MidwestAire
Copy link
Owner

yeah, I would probably maybe have to add a bit for ap_status |= (uint8_t)(AP_Notify::flags.rpm)....... or similar. Although don't the other MavLink messages come thru on the message display screen and sound the buzzer for CRT warnings? Such as CRT: Compass variance or CRT: Bad AHRS. Or do those have to be hard-coded into the FrSky passthru?

@MidwestAire
Copy link
Owner

I think I would prefer to put that in the AP_Frsky_Telem::check_sensor_status_flags() function? Then it displays a message on the bottom of the screen and sounds the warning buzzer. I know from experience in flight when that goes off it gets my immediate attention.

@MidwestAire
Copy link
Owner

Or maybe I don't have to do anything with the FrSky library. I have custom messages that already come over the MavLink stream and sound the warning buzzer.

100_0202

@yaapu
Copy link
Contributor Author

yaapu commented Dec 9, 2019

@ChristopherOlson both would work, plain text messages and check_sensor_status_flags() customization, from the script point of view they are both simple text messages, the script will display them and play the warning sound when required, you could also generate custom vocal sounds to play for each message without touching the script implementation.
This will warn the user but won't trigger anything in the script kind of like the "reached waypoint.." vocal event

@MidwestAire
Copy link
Owner

Right. I think the normal MavLink stream text message like I have with the Throttle Hold will work fine. I can do that in the main code, I don't think a spoken message is needed. Just something to get the pilot's attention so he/she looks at the instrument panel to identify the problem. If the rpm sensor is zero that is impossible - we likely have a failed sensor. If it's 1,400 rpm the pilot has a real autorotation emergency on her hands and she better get out of the altitude controlled flight mode ASAP before it stalls the rotor.

Having this on the RC where the pilot controls the helicopter is invaluable. Having it on the GCS is useless because of the pilot splitting her attention between trying to see a computer screen that is likely not in front of her vs the actual controls, and it delays the decision making process. The GCS is like trying to drive your car with the instrument panel in the back seat.

@MidwestAire
Copy link
Owner

@yaapu if you make more changes to this fetch master and rebase to fix the conflicts. I'll be pushing more commits to master to add support for twin-engine throttle curves.

@yaapu
Copy link
Contributor Author

yaapu commented Dec 10, 2019

@ChristopherOlson yes no problem

…sstrough link

Copter: added new constructor to pass a pointer for the rpm sensor library
Frsky: added encoding of the rpm info as 16bit unsigned int and updated the scheduler to support packet type 0x50A0
@yaapu
Copy link
Contributor Author

yaapu commented Dec 17, 2019

@ChristopherOlson the last commit includes support for the vfrhud message in order to get the throttle value.

this is the companion helipilot version of my script.
Enter the menu and select "helipilot" for the right panel.

image

yaapu_helipilot_1.8.1_dev.zip

Note: I did test it in SITL and trying to takeoff in acro I got the "rc value too high" message :-)

@MidwestAire
Copy link
Owner

@yaapu this is cool! Is the throttle the actual Heli Throttle (engine) output, or the collective output?

@yaapu
Copy link
Contributor Author

yaapu commented Dec 18, 2019

Hi Chris, I have to admit I have no clue about collective vs throttle, it's the same value you'd see in mavproxy in the console module as thr and in the vfr HUD message as throttle

@MidwestAire
Copy link
Owner

Ah, ok. That is an anomaly in the code. Throttle in helicopters refers to engine throttle and thrust is changed by changing collective pitch. But all thru the code in the attitude controller, etc, etc the helicopter collective is referred to as "throttle" like it would be with a multicopter. Except in the heli-specific code where we have proper collective and throttle terminology.

So that throttle % is really collective percent in a helicopter, which is displayed on the instrument panel in most heli's as % torque because helicopters are a constant speed system with torque load directly related to percentage of blade pitch.

Probably more than you wanted to know about the situation :-)

@yaapu
Copy link
Contributor Author

yaapu commented Dec 18, 2019

Probably more than you wanted to know about the situation :-)

Chris I do have and fly albeit not very well a flybarred TRex-250, what I meant is that I had no clues on how arducopter refers to throttle vs collective pitch.
From your explanation throttle from vfrhud is collective pitch and should be what you were after.

Let me know if you get a chance to test both helipilot+script!

EDIT: should we change the label for throttle from THR to CP?

@Aris2024
Copy link

Hi Guys, just to help a little bit here:
Regarding the helicopters and helipilot FW, Ch3 is assigned to the collective pitch. Helicopters are flying with fixed RPMs and they are increasing or decreasing the blades pitch in order to ascent or descent.
Ch8 is assigned to throttle. So the RPM sensor and the governor can all influence the ch8 output that is the motor's throttle.
The confusion comes from the hobby grade helicopters where they use to mix the pitch and the throttle together. So on these by moving Ch3 (usually left stick up - down) you are not only changing the pitch but also the throttle output. Then quadcopters and arducopter that appeared after the helicopters where already using that, kept using channel 3 as throttle only. They cannot change any pitch anyway. And from all the above on arducopter they keep referring to ch3 as throttle which is wrong. Ch3 is normally the collective pitch on the helicopters.

@MidwestAire
Copy link
Owner

EDIT: should we change the label for throttle from THR to CP?

Actually for helicopters just the name on the instrument panel should be named % Torque. That's what it is in the Cabri G2, for instance. Move the collective lever without the helicopter even running and you can watch the % Torque gauge on the panel go from 0 to 100% :-)

That's the nice thing about helicopters is that torque is directly correlated to collective pitch because they are a constant-speed system!

@MidwestAire
Copy link
Owner

MidwestAire commented Dec 18, 2019

And from all the above on arducopter they keep referring to ch3 as throttle which is wrong. Ch3 is normally the collective pitch on the helicopters.

@Aris2024 I actually have most of this fixed in HeliPIlot. There is still a few areas in the code that need attention, especially in the attitude controller. But in HeliPilot the pilot or technician should no longer be addressed using "throttle" where collective is concerned (as long as you use the QGC build for HeliPilot). For instance, try to arm with the throttle off the bottom stop in HeliPilot, you will get a "Check Throttle Position" message. In ArduPilot you will get "Motor Interlock Enabled". Another example is try to arm with the collective over 50% and in HeliPilot you will get "Collective too high" message. In ArduPilot you will get "throttle too high" message.

In HeliPilot I even threw the old "RSC" into the garbage can and replaced it with a brand new Throttle control library to get the terms "collective" and "throttle" meaning what they are supposed to mean in helicopters.

In ArduPilot I could never do this, as it broke compatibility with multicopters. In HeliPilot we don't care about multicopters.

@yaapu
Copy link
Contributor Author

yaapu commented Dec 18, 2019

I like "torque"

image

@MidwestAire
Copy link
Owner

@yaapu Yes, I like that too. Torque is proper term.

I am still putting the finishing touches on the new torque limiter in the Throttle library in HeliPilot. As soon as I get that all done, then I will test this entire system as a package. This is looking really good!

@MidwestAire
Copy link
Owner

@yaapu the lua script definitely works in the radio. If I had the helipilot right panel on the altview with the rest of my external sensors it would be perfect.

I changed mine to "torque" too, before I loaded it in the radio :-)

I'll have to pull your pr-frsky_rpm branch and cherry-pick it into master, build it and load it in one of the 766's to try it out. Can start one of those up this weekend to put the test to it. It's below zero here. Which is a different temperature scale than you use in Europe. They base a temp of zero on when water freezes in Europe. Here in the U.S. a temp of zero is based on when JetA freezes :-)

@MidwestAire
Copy link
Owner

@yaapu I got a chance to load the build into a 766. I get telemetry from the external sensors. But no passthru at all. I didn't get time to diagnose why. It may have something to do with the scheduler commit as I can't see anything in this PR that would cause the passthru to not work.

@yaapu
Copy link
Contributor Author

yaapu commented Dec 19, 2019

I'll try my branch on a spare Pixhawk1 later today

@MidwestAire
Copy link
Owner

I'll get a chance to do some diagnostics this evening, as well.

@MidwestAire
Copy link
Owner

@yaapu I reverted to HeliPilot 19 and then the script works fine, although no RPM or Torque. I just changed the display name for "throttle" in the helipilot right panel one. What is the m2f script for? I see that one has "throttle" added to it also.

I am using a NuttX build with v20-dev, I didn't try ChibiOS build. Our controllers can run either RTOS. That shouldn't make a difference, but I might try a ChibiOS build in case the scheduler is causing the problem.

@yaapu
Copy link
Contributor Author

yaapu commented Dec 19, 2019

What is the m2f script for? I see that one has "throttle" added to it also.

for the mavlinkToPassthru firmware running on a separate board.
It supports waypoints, native airspeed, baro altitude and throttle

@yaapu
Copy link
Contributor Author

yaapu commented Dec 19, 2019

I am using a NuttX build with v20-dev, I didn't try ChibiOS build. Our controllers can run either RTOS. That shouldn't make a difference, but I might try a ChibiOS build in case the scheduler is causing the problem.

could be, this version incorporates a PR to use a dedicated thread for the scheduler, it might just not work in NuttX

@MidwestAire
Copy link
Owner

could be, this version incorporates a PR to use a dedicated thread for the scheduler, it might just not work in NuttX

That's what I'm thinking. I don't see a problem in the code. I will try a ChibiOS build this evening and see if it works, as there is significant differences between the scheduler in the two RTOS's.

@yaapu
Copy link
Contributor Author

yaapu commented Dec 19, 2019

as there is significant differences between the scheduler in the two RTOS's.

If it does not work we can revert to the default 3.9.x threading while retaining the new WFQ scheduler

@MidwestAire
Copy link
Owner

https://discuss.ardupilot.org/t/follow-up-on-your-post/50310

I have a gut instinct it is going to work on ChibiOS. Let me try that first this evening, as the high-performance scheduler and threading in the ChibiOS kernel does have some advantages. I will report back after I try it. It is just a matter of doing a fmuv3 build instead of px4-v3.

@MidwestAire
Copy link
Owner

@yaapu it works positively beautiful with ChibiOS

100_0210

Especially the RPM. I'm not sure what the stream rate of that is to Mission Planner. But on a test here the RPM on the radio is anywhere from ~2.5 seconds to 7 seconds ahead of Mission Planner's rpm display on the HUD. It must make a difference as to what else is coming down the MavLink pipe as to how fast Mission Planner will update that. The RPM library was only designed for logging, not RPM display. For heli, when I built the governor, I increased the scheduler rate to 40Hz for the rpm because I wasn't getting enough samples, even in the main code, to operate the governor.

So this beats the GCS hands-down on speed.

Nice work!

@MidwestAire
Copy link
Owner

@yaapu after playing with this a bit more I actually like this display. It is less cluttered, can switch to the alt view with a press of the buttons. And use audio warnings for the other screen that is not normally used in flight, but is used for engine warmup/cooldown, etc..

My feeling is that if the right panel was attempted to be combined with the altview it would become too cluttered. Ergonomically, this works better. We have monitoring of the generator, torque (which is EXTREMELY important to monitor load on the engine(s), rotor rpm. These are the critical items the pilot always watches. With the ease of switching screens to check out an audio warning for one of the other sensors, this makes sense.

I like it!

@MidwestAire
Copy link
Owner

@yaapu I am going to merge this PR, as after testing it for about an hour and a half late this afternoon I can't find anything wrong with it. I want to rebase master on Copter to get the ChibiOS fixes, especially for the I2C storm issues. Having this merged on top before I rebase will be better, as there will be some conflicts. I did a test rebase and it will take a me a bit to fix the conflicts, better me to do it than causing you to have to rebase to keep this PR up-to-date :-)

@MidwestAire MidwestAire merged commit 1187ee4 into MidwestAire:HeliPilot-master Dec 20, 2019
@yaapu
Copy link
Contributor Author

yaapu commented Dec 20, 2019

@ChristopherOlson my plan is to let you test the usability of the UI and eventually port it to QX7 and Horus. As for merging before/after the rebase it'up to you, rebasing my PR on latest 3.9 copter should be easy.
Once you switch to the 4.0 branch it will take quite a rewrite

@MidwestAire
Copy link
Owner

@yaapu indeed your PR was not the problem. The heli code is significantly diverged from ArduPilot. I had put the ChibiOS builds on the back burner as ChibiOS was having some teething pains. I think over the last six months ChibiOS has become more mature with many issues solved. Keeping the HeliPilot specific changes at the top of the list thru rebase is good so those changes don't get lost in conflict resolution.

The 4.0 base is not on the itinerary at present because there is no functional changes in 4.0 that HeliPilot doesn't already have. Especially the changes to WPNAV that we have in HeliPilot can't be used in Copter because it breaks compatibility with multicopters. HeliPilot is way ahead of 4.0 in helicopter development, Bill will be pulling some of the changes in HeliPilot for Copter 4.1

So now that I have this merged it will be automatically included in any builds for testing to bring HeliPilot 20 version to stable status where it won't change for at least a year. The commercial folks need the stability and something that "just works" and aren't interested in constantly updating with point releases every 2-3 weeks to get the latest bug fix in Copter "stable". That's why we're based on 3.6 (at present) where the code has been thoroughly vetted and flown for at least a year.

I determined this PR to be functional. Now that it's merged and automatically included in v20 builds the real testing can start :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants