[RFC] Make NuttX drivers cross platform (VDev -> CDev) and MS4525 linux driver #7864

dagar · 2017-08-24T20:14:53Z

On NuttX we have a large collection of well tested device drivers (I2C/SPI/serial) implemented using CDev (nuttx character devices). On non-nuttx posix platforms we have a nearly identical interface called VDev which uses virtual files/devices/ioctls.

If we combine them and instead treat VDev as the platform independent version of CDev we can fairly easily "port" our existing NuttX to the other platforms.

In this PR I've renamed VDev -> CDev and ported the NuttX ms4525 differential pressure sensor driver. I've successfully tested it on a Navio2 Raspberrypi. Porting consisted of removing unnecessary headers, prepending px4_ to ioctl, open, close, and making sure the ms4525 launcher didn't call exit.

I propose we consider this as a path forward for unified cross platform PX4 drivers. My next step is adding a Linux CDev SPI interface, then porting the bulk of the important drivers. I'd like to continue the Driver rearchitecture discussion, but doing so incrementally from CDev. I have another PR that cleanly combines the Device and CDev classes between NuttX and Linux, then each have separate I2C and SPI interfaces.

@davids5 @mcharleb @bgat @LorenzMeier

TSC21 · 2017-08-24T20:18:41Z

@dagar did you see any differences in terms of performance after running top?

LorenzMeier · 2017-08-24T20:19:10Z

Awesome! Originally we did bite the bullet on Driverframework as we were hoping for more stacks and verticals / users to join the effort. That never materialized, so it might be time to reconsider.

dagar · 2017-08-24T20:57:04Z

If there's any chance that might matter I don't think it would be that hard to keep the PX4 portion (uORB and params) separate. I'd be fine supporting whichever framework (cross platform CDev descendant or DriverFramework) gets us to unified development effort and driver base as soon as possible.

davids5 · 2017-08-24T21:01:01Z

@dagar - This is a great step!

As we discussed: Once we have the SPI done, the API for a "driver runner" can be realized in Cdev and SPI and I2C with a task per bus. Then wrapping the hrt calls to as this->schedule_rt(...) should allow us to pass a config, that the implementations can then provide the best way to get the driver scheduled.
(I.E. DRDY, HRT or delayed) The net result will be either for HRT and DRDY an ISR->sem_post or a sw timer to sem_post. That should give us better realtime response (less interrupt off time) and the ability to do DMA on SPI. At the same time the other platforms can just continue to schedule the cycles as is.

bkueng

Very cool!
One key difference in functionality between CDev and VDev is that NuttX can handle Interrupts. But I only see that being used in the hc_sr04 sonar driver.

bkueng · 2017-08-25T06:11:37Z

src/drivers/ms4525_airspeed/ms4525_airspeed.cpp

@@ -417,7 +400,7 @@ start(int i2c_bus)
 	int fd;

 	if (g_dev != nullptr) {
-		errx(1, "already started");
+		PX4_ERR("already started");


Be aware that errx exited the task, so exchanging it with PX4_ERR is usually not enough.

Thanks, I'll take a closer look. One of my goals for the drivers is to remove the boiler plate that's manually duplicated across nearly all of these. Either a Driver ModuleBase or a shared launcher/driver runner.

bgat · 2017-08-25T10:21:28Z

One key difference in functionality between CDev and VDev is that NuttX

can handle Interrupts. For chips that depend on interrupts, they've probably got an in-kernel Linux driver already. If not, some kernel configurations let you reflect interrupt events out to user applications through the gpiolib interface---provided the platform is wired that way. For chips with an in-kernel Linux driver, their interfaces will likely be chip-specific. There won't be any uniform way to harmonize them with NuttX. :-( Still, I'll take 99% success. Great contribution, Daniel! :-) b.g.

On Fri, Aug 25, 2017 at 1:14 AM Beat Küng ***@***.***> wrote: ***@***.**** commented on this pull request. Very cool! One key difference in functionality between CDev and VDev is that NuttX can handle Interrupts. But I only see that being used in the hc_sr04 sonar driver. ------------------------------ In src/drivers/ms4525_airspeed/ms4525_airspeed.cpp <#7864 (comment)>: > @@ -417,7 +400,7 @@ start(int i2c_bus) int fd; if (g_dev != nullptr) { - errx(1, "already started"); + PX4_ERR("already started"); Be aware that errx exited the task, so exchanging it with PX4_ERR is usually not enough. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7864 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWp8ACCoLIR5vHnYgPr4JsJ0XTL9ySzks5sbmYngaJpZM4PB4WO> .

-- Bill Gatliff Embedded Linux training and consulting bgat@billgatliff.com (309) 453-3421

dagar · 2017-08-25T13:47:39Z

Actually I don't think any of these drivers are currently using interrupts. In the hr_sr04 case they set irq to 0.
We might be changing that soon though.

LorenzMeier · 2017-08-27T20:00:21Z

@dagar What are the steps required to get this in? Could you advise @PX4TestFlights what and how to test?

dagar · 2017-08-27T23:12:27Z

This PR is actually quite mundane, with hardly any functional change for existing usage. The new functionality is the nuttx differential pressure sensor drivers now work on linux. Some basic bench testing of a pixhawk setup with an airspeed sensor should be sufficient to ensure there aren't regressions.

The bigger question is if everyone is on board with this direction for unified drivers. There's a lot more detail to go into, but the core of it is merging the driver Device interface with I2C and SPI implementations for each platform. CDev/VDev merge to facilitate this, but that can soon be broken out as a separate optional interface. Then we gradually remove CDev usage from most drivers. Some things will still be CDev where appropriate (each orb topic, the iridiumsbd driver), but the majority of drivers won't need to be.

@mcharleb @davids5 @bgat @LorenzMeier Should we discuss this on a call?

bgat · 2017-08-27T23:37:48Z

I'm ok with that, if it can wait until Tuesday.

On Sun, Aug 27, 2017 at 6:12 PM Daniel Agar ***@***.***> wrote: This PR is actually quite mundane, with hardly any functional change for existing usage. The new functionality is the nuttx differential pressure sensor drivers now work on linux. Some basic bench testing of a pixhawk setup with an airspeed sensor should be sufficient to ensure there aren't regressions. The bigger question is if everyone is on board with this direction for unified drivers. There's a lot more detail to go into, but the core of it is merging the driver Device interface with I2C and SPI implementations for each platform. CDev/VDev merge to facilitate this, but that can soon be broken out as a separate optional interface. Then we gradually remove CDev usage from most drivers. Some things will still be CDev where appropriate (each orb topic, the iridiumsbd driver), but the majority of drivers won't need to be. @mcharleb <https://github.com/mcharleb> @davids5 <https://github.com/davids5> @bgat <https://github.com/bgat> @LorenzMeier <https://github.com/lorenzmeier> Should we discuss this on a call? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7864 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWp8CJG-PlEca-ORduDhsOrbZrUOTr7ks5scffjgaJpZM4PB4WO> .

-- Bill Gatliff Embedded Linux training and consulting bgat@billgatliff.com (309) 453-3421

davids5 · 2017-08-28T16:30:29Z

src/drivers/ms4525_airspeed/ms4525_airspeed.cpp

-	warnx("\t-b --bus i2cbus (%d)", PX4_I2C_BUS_DEFAULT);
-	warnx("command:");
-	warnx("\tstart|stop|reset|test|info");
+	PX4_INFO("usage: meas_airspeed command [options]");


@dagar Do you think it would be good to establish the new set of "good driver" habits to use the module macros here?

Yes although I'm more interested in doing in centrally (launcher or interface) and then deleting all of this code 😄.
Start with some notes for what you have in mind?

davids5 · 2017-08-28T16:33:42Z

@mcharleb @davids5 @bgat @LorenzMeier Should we discuss this on a call?

Yes. Would you like to do it sooner than just after then next dev call?

Make NuttX drivers cross platform (VDev -> CDev)

f853908

dagar added the Admin: Enhancement (improvement) 💡 label Aug 24, 2017

dagar mentioned this pull request Aug 24, 2017

Rename VDev to CDev #7817

Closed

dagar self-assigned this Aug 24, 2017

dagar added 2 commits August 24, 2017 20:59

bebop add PX4 I2C defines

da944f7

eagle add I2C defines

e3bdb27

dagar force-pushed the pr-cdev_linux_ms4525 branch from bb54e28 to e3bdb27 Compare August 25, 2017 01:00

dagar added 4 commits August 24, 2017 21:28

ms5525_airspeed posix port

0986434

sdp3x_airspeed posix port

3c4dee8

ms4525_airspeed update test helpers

78c0419

ets_airspeed posix port

6f0d770

dagar force-pushed the pr-cdev_linux_ms4525 branch from 3e7cc1c to 6f0d770 Compare August 25, 2017 01:57

bkueng reviewed Aug 25, 2017

View reviewed changes

LorenzMeier approved these changes Aug 27, 2017

View reviewed changes

davids5 reviewed Aug 28, 2017

View reviewed changes

nicolaerosia mentioned this pull request Aug 30, 2017

WIP: Linux mpu9250 rework #7505

Closed

dagar mentioned this pull request Aug 30, 2017

Add POSIX support for Aerotenna ulanding radar #7788

Merged

LorenzMeier merged commit fd8a564 into PX4:master Aug 31, 2017

dagar deleted the pr-cdev_linux_ms4525 branch August 31, 2017 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Make NuttX drivers cross platform (VDev -> CDev) and MS4525 linux driver #7864

[RFC] Make NuttX drivers cross platform (VDev -> CDev) and MS4525 linux driver #7864

dagar commented Aug 24, 2017 •

edited

Loading

TSC21 commented Aug 24, 2017

LorenzMeier commented Aug 24, 2017

dagar commented Aug 24, 2017

davids5 commented Aug 24, 2017 •

edited

Loading

bkueng left a comment

bkueng Aug 25, 2017

dagar Aug 25, 2017 •

edited

Loading

bgat commented Aug 25, 2017 via email

dagar commented Aug 25, 2017

LorenzMeier commented Aug 27, 2017

dagar commented Aug 27, 2017

bgat commented Aug 27, 2017 via email

davids5 Aug 28, 2017

dagar Aug 28, 2017

davids5 commented Aug 28, 2017

[RFC] Make NuttX drivers cross platform (VDev -> CDev) and MS4525 linux driver #7864

[RFC] Make NuttX drivers cross platform (VDev -> CDev) and MS4525 linux driver #7864

Conversation

dagar commented Aug 24, 2017 • edited Loading

TSC21 commented Aug 24, 2017

LorenzMeier commented Aug 24, 2017

dagar commented Aug 24, 2017

davids5 commented Aug 24, 2017 • edited Loading

bkueng left a comment

Choose a reason for hiding this comment

bkueng Aug 25, 2017

Choose a reason for hiding this comment

dagar Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

bgat commented Aug 25, 2017 via email

dagar commented Aug 25, 2017

LorenzMeier commented Aug 27, 2017

dagar commented Aug 27, 2017

bgat commented Aug 27, 2017 via email

davids5 Aug 28, 2017

Choose a reason for hiding this comment

dagar Aug 28, 2017

Choose a reason for hiding this comment

davids5 commented Aug 28, 2017

dagar commented Aug 24, 2017 •

edited

Loading

davids5 commented Aug 24, 2017 •

edited

Loading

dagar Aug 25, 2017 •

edited

Loading