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

Introduce new generic ADC driver for ADS1115 chip #14437

Merged
merged 16 commits into from
Sep 14, 2020

Conversation

SalimTerryLi
Copy link
Contributor

Describe problem solved by this pull request
This driver would provide ADC interface for boards without on-chip ADC function such as RPi.

Describe your solution
Sampling on trigger, SPS=250, VREF=6.144.
Scheduled at 80Hz, looping through its four multiplexed channel, the driver publishes a new adc_report after all 4 channels are captured, at 20Hz.

This driver is designed to run on any platforms. But ADC subscribers are not checking devID at now, so it should only run on boards without on-chip ADC.

Test data / coverage
Works fine on RPi.
图片

@bresch bresch added the Drivers 🔧 Sensors, Actuators, etc label Mar 20, 2020
@SalimTerryLi SalimTerryLi force-pushed the pr-ads1115_driver-new branch 3 times, most recently from fa8db9c to ccd1722 Compare March 24, 2020 20:32
@dagar dagar self-requested a review March 25, 2020 04:42
@dagar
Copy link
Member

dagar commented Mar 25, 2020

I don't quite understand the need for the I2C_Interface and the ADS1115_Wrapper. Why not just have a simple ADS1115 that extends device::I2C?

Example: https://github.com/PX4/Firmware/blob/master/src/drivers/magnetometer/isentek/ist8308/IST8308.hpp

The other minor thing is the placement within another module (drivers/adc). I'd like to avoid modules within modules, although obviously our organization system needs work (where to put or name the generic adc driver).

@SalimTerryLi
Copy link
Contributor Author

SalimTerryLi commented Mar 25, 2020

I don't quite understand the need for the I2C_Interface and the ADS1115_Wrapper. Why not just have a simple ADS1115 that extends device::I2C?

Example: https://github.com/PX4/Firmware/blob/master/src/drivers/magnetometer/isentek/ist8308/IST8308.hpp

The other minor thing is the placement within another module (drivers/adc). I'd like to avoid modules within modules, although obviously our organization system needs work (where to put or name the generic adc driver).

I just want to make it more clear to understand. ADS1115 is only a logic driver and the wrapper do the other things like device registering and scheduling. PCA9685(#14269) is also designed this way.
At least I want to do the PX4 specified scheduling out of driver logic.

Seems it cannot be achieved now.... I'll migrate them into the same place

@SalimTerryLi SalimTerryLi force-pushed the pr-ads1115_driver-new branch 2 times, most recently from 962ff61 to b3b6849 Compare April 2, 2020 14:23
@SalimTerryLi SalimTerryLi force-pushed the pr-ads1115_driver-new branch from b3b6849 to 0d2bf87 Compare April 11, 2020 18:14
@stale
Copy link

stale bot commented Jul 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@SalimTerryLi
Copy link
Contributor Author

SalimTerryLi commented Aug 6, 2020

updated & checked for availability

@dagar
Copy link
Member

dagar commented Sep 7, 2020

Happy to merge once #14477 is in.

@SalimTerryLi
Copy link
Contributor Author

Happy to merge once #14477 is in.

Ready.

@@ -19,6 +19,7 @@ px4_add_board(
TEL4:/dev/ttyS6
DRIVERS
adc/board_adc
adc/ADS1115
Copy link
Member

Choose a reason for hiding this comment

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

This is nitpicky, but for consistency how about keeping the folder name lowercase like all the other drivers and modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed.

@dagar dagar merged commit 28a6e82 into PX4:master Sep 14, 2020
Copy link

@ibrhm0v ibrhm0v left a comment

Choose a reason for hiding this comment

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

Everything seems ok to me @SalimTerryLi great work.
Update: The only issue I have faced testing your driver is when the driver is already running and is started again, it crashes and the driver can never be started again unless the autopilot is rebooted.

@SalimTerryLi
Copy link
Contributor Author

SalimTerryLi commented Oct 15, 2020

Everything seems ok to me @SalimTerryLi great work. The only issue I have faced testing your driver is when the driver is already running and is started again, it crashes and the driver can never be started again unless the autopilot is rebooted.

Oh yes. It seems I forget to do ScheduleClear() on exit. I will test and fix it later. Thanks for your report.
Update: This comment is wrong.

@SalimTerryLi
Copy link
Contributor Author

Everything seems ok to me @SalimTerryLi great work. The only issue I have faced testing your driver is when the driver is already running and is started again, it crashes and the driver can never be started again unless the autopilot is rebooted.

Could you please provide more information? How it crashed? Anything outputted? It works just fine on my Pi. BTW there is no need to manually call ScheduleClear();, as it is done by I2CSPIDriverBase.

pxh> ads1115 start -I
WARN  [SPI_I2C] Already running on bus 1
WARN  [SPI_I2C] UnknownApp: no instance started (no device on bus?)
Command 'ads1115' failed, returned -1.
pxh> ads1115 start -I
WARN  [SPI_I2C] Already running on bus 1
WARN  [SPI_I2C] UnknownApp: no instance started (no device on bus?)
Command 'ads1115' failed, returned -1.
pxh> ads1115 INFO  [ecl/EKF] 624834243: EKF aligned, (baro hgt, IMU buf: 18, OBS buf: 14)
pxh> ads1115 stop
INFO  [ads1115] stopping
pxh> ads1115 stop
ERROR [SPI_I2C] Not running
Command 'ads1115' failed, returned -1.
pxh> ads1115 start -I
ads1115 #0 on I2C bus 1
pxh> ads1115 start -I
WARN  [SPI_I2C] Already running on bus 1
WARN  [SPI_I2C] UnknownApp: no instance started (no device on bus?)
Command 'ads1115' failed, returned -1.
pxh> ads1115 stop
INFO  [ads1115] stopping

@ibrhm0v
Copy link

ibrhm0v commented Oct 15, 2020

Oh ok it is my bad. I am working on my customized firmware that is still back at 1.9.0. Therefore I have ammended your main function. My start() implementation was wrong. I have fixed it now.

@SalimTerryLi SalimTerryLi deleted the pr-ads1115_driver-new branch April 24, 2023 06:37
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.

5 participants