Refactor cm8jl65 driver. Employ uniform initialization, format, and deprecate ringbuffer and IOCTL. #11853

mcsauder · 2019-04-15T17:51:23Z

Describe problem solved by the proposed pull request
This PR refactors the CM8JL65 driver to employ uniform initialization, move the parser and crc16 methods into the CM8JL65 class, move doxy documentation to the method declarations, and formats/standardize the driver structure.

This PR is a step in standardizing the cm8jl65 driver with other distance sensor drivers moving toward resolution of Issue #9279.

Distance sensor drivers and IMU drivers are perhaps the most numerous instances of code duplication in the drivers. Similar work is being undertaken in the IMU drivers as well, (PR #11629). Additional work within this driver will follow this PR.

Additional context
See Issue #9279.

Test data / coverage
This PR has been bench tested and flight tested with pixhawk 4 and pixkhawk 4 mini hardware, see comments below for data plots.

Please let me know if you have questions on this PR. Thanks!

-Mark

mcsauder · 2019-04-30T18:51:20Z

@cmic0 , would you mind reviewing this PR? Thanks in advance!

dagar · 2019-06-16T00:51:26Z

src/drivers/distance_sensor/cm8jl65/cm8jl65.cpp


-};
+	ringbuffer::RingBuffer *_reports{nullptr};


Can this be gutted entirely (ringbuffer, ioctl, read, etc)?

Unfortunately, not that easily... See my work here. However, if we can get this PR accepted and one to follow it from the continued work in the link I think the issue will become apparent.

I can successfully deprecate IOCTL usage from I2C devices, but not CDev devices. For CDev devices, these calls are still required.

UPDATED: The ringbuffer and read calls might be a little more straightforward. I will address those in subsequent PRs.

Please disregard my comments above. I discovered the issue preventing me from succeeding in deleting the ioctl calls was that the ioctl calls were where the start() method was being called from. Palm->forehead. :}

I have pushed up a commit that is bench tested and accomplishes your requests. I will post a/b comparisons and flight logs as soon as I can get to that work.

mcsauder · 2019-06-16T23:54:27Z

Hi @cmic0 and @dagar , here are flight logs and plots of current PX4 master and this PR on a 250 sized drone, pixhawk 4 mini in position hold mode from ground level incrementing altitude slowly and holding at ~75cm increments up to ~3m.. The results are indistinguishable to me, so I think this PR is good to go. (I would like to squash the commits before merging to keep a clean history, please let me know when I can do this.)

PX4 master: https://review.px4.io/plot_app?log=89b6ccba-833f-454f-a0a7-f82078fbb049

This PR: https://review.px4.io/plot_app?log=b09b4f8a-12d4-4d1a-a062-a51852f3b189

mcsauder · 2019-06-24T15:35:07Z

@dagar, ping.

mcsauder · 2019-06-28T04:38:49Z

@dagar and @davids5 , the cm8jl65 driver has sufficient stack allocated in PX4:master and (unchanged) with this PR:

davids5 · 2019-06-28T14:55:26Z

@mcsauder - we need @dagar confirmation but (IIRC) the 300 margin in the stackmon could be reduced to 96-100 - we only stack like 8 registers on the user process stack when an ISR is run. The rest of the penetration is application determined. Without 100% code coverage that worst case is not known. But 96 bytes of overhead is more than enough if the longest call depth has been exercised.

…st to declarations. Format whitespace and move method doxy comments to declarations.

- Condense all class files into a single *.cpp file and give class scope resolution to previously unscoped methods. - Refactor cm8jl65 namespace level driver entry methods to reduce code and improve clarity. - Breakout CDev specific initialization LOC into the device_init() method. - Move the endian modification inside of the crc16_calc() method.

…the cm8jl65 driver.

… printout and correct a comment in the driver.

mcsauder · 2019-06-28T16:20:56Z

@davids5 and @dagar , I've reduced the cm8jl65 stack allocation from 1400 to 1200, merged latest master and rebased.

Stackcheck fails at 1100 and functions at the current commit value of 1200.

davids5 · 2019-06-28T17:29:40Z

@mcsauder There are 3 things here.

The stack check build - hardware test from 68+136 margin
The loadmon's opinion that 300 (iirc) are needed.
The Actual penetration of the application + ISR overhead.

The hard truth is that #1 will dictate 204 bytes of margin (it does not assume a separate interrupt stack)
So it is wasteful.

So I would have stack mon warn at 216

mcsauder · 2019-06-29T02:01:33Z

Thank you for your review @dagar !

mcsauder · 2019-06-29T02:02:02Z

(and thank you for your feedback @davids5!!!)

mcsauder force-pushed the cm8jl65_driver_work branch from 63500ef to 9a54c3c Compare April 15, 2019 19:15

mcsauder force-pushed the cm8jl65_driver_work branch 3 times, most recently from e0f1e59 to e929642 Compare April 23, 2019 15:19

mcsauder changed the title ~~Move cm8jl65 variable initialization from constructor list to declarations, format whitespace/comments.~~ Migrate cm8jl65 variable initialization from constructor list to declarations, format whitespace/comments. Apr 24, 2019

mcsauder force-pushed the cm8jl65_driver_work branch 2 times, most recently from d0d9f3a to 719df19 Compare April 30, 2019 15:47

mcsauder force-pushed the cm8jl65_driver_work branch 6 times, most recently from 569a28a to 5bdbbbd Compare May 3, 2019 15:00

weekly-digest bot mentioned this pull request May 5, 2019

Weekly Digest (28 April, 2019 - 5 May, 2019) #11965

Closed

mcsauder force-pushed the cm8jl65_driver_work branch from 5bdbbbd to d5d6b4c Compare May 6, 2019 18:58

mcsauder mentioned this pull request May 6, 2019

[WIP] Create a distance sensor driver base class #11977

Closed

14 tasks

mcsauder force-pushed the cm8jl65_driver_work branch 2 times, most recently from 4b1fc26 to b6f0b3a Compare May 10, 2019 03:30

weekly-digest bot mentioned this pull request May 12, 2019

Weekly Digest (5 May, 2019 - 12 May, 2019) #12005

Closed

mcsauder force-pushed the cm8jl65_driver_work branch from b6f0b3a to 96a7a77 Compare May 14, 2019 14:47

weekly-digest bot mentioned this pull request May 19, 2019

Weekly Digest (12 May, 2019 - 19 May, 2019) #12043

Closed

dagar reviewed Jun 16, 2019

View reviewed changes

mcsauder force-pushed the cm8jl65_driver_work branch from 8fe329e to f5e63d1 Compare June 16, 2019 23:10

mcsauder changed the title ~~Migrate cm8jl65 variable initialization from constructor list to declarations, format whitespace/comments.~~ Migrate cm8jl65 variable initialization from constructor list to declarations, format whitespace/comments and deprecate IOCTL. Jun 16, 2019

mcsauder force-pushed the cm8jl65_driver_work branch from f5e63d1 to 640de03 Compare June 19, 2019 15:41

weekly-digest bot mentioned this pull request Jun 23, 2019

Weekly Digest (16 June, 2019 - 23 June, 2019) #12318

Closed

mcsauder mentioned this pull request Jun 24, 2019

Drivers Code Cleanup #9279

Closed

mcsauder force-pushed the cm8jl65_driver_work branch 3 times, most recently from c17f0b9 to eb2bc54 Compare June 28, 2019 03:20

mcsauder added 5 commits June 28, 2019 10:18

Move cm8jl65 class member variable initialization from constructor li…

93a5c84

…st to declarations. Format whitespace and move method doxy comments to declarations.

Refactor the cm8jl65 driver start() method, and rename info()->status().

5b78a12

Deprecate the hardware ringbuffer, _class_instance, and ioctl() from …

3ae737d

…the cm8jl65 driver.

Reduce the stack size allocated for the cm8jl65 driver, comment out a…

a5a81c0

… printout and correct a comment in the driver.

mcsauder force-pushed the cm8jl65_driver_work branch from eb2bc54 to a5a81c0 Compare June 28, 2019 16:18

mcsauder changed the title ~~Migrate cm8jl65 variable initialization from constructor list to declarations, format whitespace/comments, refactor, and deprecate IOCTL.~~ Refactor cm8jl65 driver. Employ uniform initialization, format, and deprecate ringbuffer and IOCTL. Jun 28, 2019

dagar approved these changes Jun 29, 2019

View reviewed changes

dagar merged commit db96b6c into PX4:master Jun 29, 2019

mcsauder deleted the cm8jl65_driver_work branch June 29, 2019 02:00

weekly-digest bot mentioned this pull request Jun 30, 2019

Weekly Digest (23 June, 2019 - 30 June, 2019) #12374

Closed

Jaeyoung-Lim mentioned this pull request Mar 24, 2021

Port si7210 driver for updated PX4 ethz-asl/ethzasl_fw_px4#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor cm8jl65 driver. Employ uniform initialization, format, and deprecate ringbuffer and IOCTL. #11853

Refactor cm8jl65 driver. Employ uniform initialization, format, and deprecate ringbuffer and IOCTL. #11853

mcsauder commented Apr 15, 2019 •

edited

mcsauder commented Apr 30, 2019

dagar Jun 16, 2019

mcsauder Jun 16, 2019 •

edited

mcsauder Jun 16, 2019

mcsauder commented Jun 16, 2019

mcsauder commented Jun 24, 2019

mcsauder commented Jun 28, 2019

davids5 commented Jun 28, 2019

mcsauder commented Jun 28, 2019

davids5 commented Jun 28, 2019 •

edited

mcsauder commented Jun 29, 2019

mcsauder commented Jun 29, 2019

Refactor cm8jl65 driver. Employ uniform initialization, format, and deprecate ringbuffer and IOCTL. #11853

Refactor cm8jl65 driver. Employ uniform initialization, format, and deprecate ringbuffer and IOCTL. #11853

Conversation

mcsauder commented Apr 15, 2019 • edited

mcsauder commented Apr 30, 2019

dagar Jun 16, 2019

Choose a reason for hiding this comment

mcsauder Jun 16, 2019 • edited

Choose a reason for hiding this comment

mcsauder Jun 16, 2019

Choose a reason for hiding this comment

mcsauder commented Jun 16, 2019

mcsauder commented Jun 24, 2019

mcsauder commented Jun 28, 2019

davids5 commented Jun 28, 2019

mcsauder commented Jun 28, 2019

davids5 commented Jun 28, 2019 • edited

mcsauder commented Jun 29, 2019

mcsauder commented Jun 29, 2019

mcsauder commented Apr 15, 2019 •

edited

mcsauder Jun 16, 2019 •

edited

davids5 commented Jun 28, 2019 •

edited