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

WIP: Aggregate RCOutput with UAVCAN to one place in SRV_Channels #7882

Merged
merged 20 commits into from
May 25, 2018

Conversation

EShamaev
Copy link
Member

@EShamaev EShamaev commented Mar 9, 2018

As support for UAVCAN output is widening, there is a need to put it in one place instead of copy-paste same code all over libraries.
Maybe this is overkill, but solves the problem.
As a side effect adds UAVCAN support to Intel Aero as it came from #7729

@anemostec
Copy link

Thanks Shamaev for this support. For the intel aero we need the RCoutput tap to be removed when the UAVCAN support is added. The reason for this is that RCouputtap uses the /dev/ttyS1 which could be used by other portions of the code, for the telemetry for instance.

@EShamaev
Copy link
Member Author

EShamaev commented Mar 9, 2018

It should work without a tap now, can you give it a try?

@anemostec
Copy link

not yet, I'm in the middle of an other development and CAN is not connected

@anemostec
Copy link

RCoutputTap should be desactivated, I know it can work without it

@EShamaev
Copy link
Member Author

EShamaev commented Mar 9, 2018

What I mean is that in this PR output is updated not in tap module but in normal RCOutput for Intel Aero. Tap is not needed and it's a change from your old PR. Please make tests when you can to make sure all is working.

@lucasdemarchi
Copy link
Contributor

@EShamaev I gave it a quick look and it's much better now. However I think the individual RCOutput implementations should not be changed to call uavcan. Instead leave it to be done by whom call them. This way it's copter/plane/etc (or AP_Motor if everybody is using it) responsibility to do it and you then don't have to change each driver.

Thanks.

@EShamaev
Copy link
Member Author

EShamaev commented Mar 9, 2018

Will it be enough to add it to SRV_Channels::push() then?

@EShamaev
Copy link
Member Author

EShamaev commented Mar 9, 2018

Actually UAVCAN can be totally separated from RCOutput and placed into SRV_Channels.

@lucasdemarchi
Copy link
Contributor

Looks like a good option. I can't check right now as I'm on a phone. Just check if it cover all the callers.

@@ -10,6 +10,8 @@
#define PCA9685_QUATENARY_ADDRESS 0x55
#define PCA9685_QUINARY_ADDRESS 0x61

#define PWM_CHAN_COUNT 16
Copy link
Contributor

Choose a reason for hiding this comment

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

this one should be put back into .cpp too

@khancyr
Copy link
Contributor

khancyr commented Mar 9, 2018

Looks good, but I am still not sure where this must be called, on lib like SRV_Channels or on vehicle code directly ? I don't know ... I got the same problem with a serial protocol for rover motor ...

@EShamaev
Copy link
Member Author

EShamaev commented Mar 9, 2018

There is no such calls left in vehicles code. All is done through SRV_Channels.

@khancyr
Copy link
Contributor

khancyr commented Mar 9, 2018

We can put that in the same place as SRV_Channels::output_ch_all() or do another lib that will handle the update of srv_chanell if enable, uavcan if enable, whatever_protocol if enable etc .
SRV_Channel is really for channel, I am not we want it to be polluted by can. But I don't have a better idea, so lets merge something and refactor atfer !

@EShamaev
Copy link
Member Author

EShamaev commented Mar 9, 2018

Is SRV_Channels a singleton?

@khancyr
Copy link
Contributor

khancyr commented Mar 9, 2018

hummmm SRV_channel update sbus and voltz protocl so, let's put uavcan here too ^^

@EShamaev
Copy link
Member Author

EShamaev commented Mar 9, 2018

Yes, that's logical place for it to be.

@EShamaev EShamaev changed the title WIP: Aggregate RCOutput with UAVCAN to one place in top HAL WIP: Aggregate RCOutput with UAVCAN to one place in SRV_Channels Mar 9, 2018
@@ -420,6 +420,7 @@ class SRV_Channels {
static void cork();

static void push();
static void push_UAVCAN(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

push_UAVCAN() should be private

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

This looks like a nice improvement. What is left to do with this? Has it been tested?

@lucasdemarchi
Copy link
Contributor

I do have boards with can, but I don't have any ESC right now with it. So, I think the only thing missing would be to test.

@tridge tridge force-pushed the UAVCAN_RCOutput_aggregate branch from 6ca3a04 to 5bcd5ca Compare May 22, 2018 02:18
@tridge
Copy link
Contributor

tridge commented May 22, 2018

I have done a forced push to the PR to rebase it on master
I've also found it has a major memory leak - it drops to zero available memory as soon as it starts up on both HAL_PX4 and HAL_ChibiOS

@tridge tridge force-pushed the UAVCAN_RCOutput_aggregate branch 3 times, most recently from 59fc9cf to 51697bf Compare May 24, 2018 23:14
EShamaev and others added 20 commits May 25, 2018 12:26
this stops us keeping messages for resend for too long, which fixes a
major memory leak
this allows for much higher ESC and servo rates, as it gives more
changes for frames to get out
this allows for servos at 50Hz while keeping ESCs at higher rates
this reduces the number of msg timeouts
this allows us to support 800Hz main loop rate with UAVCAN ESCs on
copter
this avoids the need to check return result
@tridge tridge force-pushed the UAVCAN_RCOutput_aggregate branch from da91985 to 6f82029 Compare May 25, 2018 02:26
@tridge tridge merged commit fbd80ef into ArduPilot:master May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants