Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Driver for Ads868x #860

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Driver for Ads868x #860

wants to merge 3 commits into from

Conversation

lukh
Copy link
Contributor

@lukh lukh commented May 3, 2022

Implementation for ADS868x ADC

@rleh rleh self-requested a review May 9, 2022 17:15
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Overall nice driver!

@lukh lukh force-pushed the ads868x branch 2 times, most recently from 140af48 to ba926d8 Compare June 8, 2022 07:32
@salkinium
Copy link
Member

I've squashed your commits and refactored the driver to use resumable functions. I also added a simple example.
However, since I don't have the hardware at hand, I cannot test it, so if you have the time, please test it and fix the driver if necessary. Perhaps also adapt the example to something more useful.

@lukh
Copy link
Contributor Author

lukh commented Feb 5, 2023

Thanks, I tried to do it, but I failed ;)
I two weeks I 'll be able to test it !

@lukh
Copy link
Contributor Author

lukh commented Feb 5, 2023

OOps
I did a mistake there. I don't know how I did it, it appears I force pushed my old branch again.

@salkinium
Copy link
Member

Fetch your remote first, then switch branch to the new origin branch, then you can modify you branch again.

@lukh
Copy link
Contributor Author

lukh commented Feb 23, 2023

I'll try the code next week, I got ~2 years of modm update to import the target fw. :)

Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Thanks for contributing the driver!

I think it might make sense to name the driver differently, since there are several ADCs from TI under the name ADS868x (🤦🏽‍♀️), of which only ADS8681, ADS8685 and ADS8689 (datasheet) fit with this driver.
The chips ADS8684 and ADS8688 (datasheet) have 4 and 8 channels respectively and need a different/extended driver.

@@ -0,0 +1,59 @@
/*
* Copyright (c) 2021, Raphael Lehmann
Copy link
Member

Choose a reason for hiding this comment

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

Probably not 😆

Comment on lines +14 to +20
def init(module):
module.name = ":driver:ads868x"
module.description = """
# ADS868x Driver.

ADS868x 16-Bit, High-Speed, Single-Supply, SAR ADC Data Acquisition System
With Programmable, Bipolar Input Ranges
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def init(module):
module.name = ":driver:ads868x"
module.description = """
# ADS868x Driver.
ADS868x 16-Bit, High-Speed, Single-Supply, SAR ADC Data Acquisition System
With Programmable, Bipolar Input Ranges
def init(module):
module.name = ":driver:ads8681"
module.description = """
# ADS8681/ADS8685/ADS8689 Driver.
ADS8681/ADS8685/ADS8689 16-Bit, Single-Channel, High-Speed, Single-Supply, SAR ADC Data Acquisition System
With Programmable, Bipolar Input Ranges
This driver does not work with ADS8684 and ADS8685 quad/octa channel ADCs.

Copy link
Member

Choose a reason for hiding this comment

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

@salkinium (How) is it possible with lbuild to name this driver :ads8681 and have aliases :ads8685/:ads8689?

Copy link
Member

@salkinium salkinium Feb 28, 2023

Choose a reason for hiding this comment

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

Yes, lbuild has aliases already built-in. Though in this case we probably want a silent alias, not sure if that is supported currently, or if lbuild just prints an empty warning for it still.

Comment on lines +80 to +81
modm::delay_us(1);
Cs::reset();
Copy link
Member

Choose a reason for hiding this comment

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

A non-blocking wait would be nicer here, like for example implemented here:

const modm::ShortPreciseDuration tStall{std::chrono::microseconds(16)};
modm::ShortPreciseTimeout timeout{tStall};

if (this->releaseMaster()) {
Cs::set();
}
timeout.restart(tStall);
RF_WAIT_UNTIL(timeout.isExpired());
RF_WAIT_UNTIL(this->acquireMaster());
Cs::reset();

Comment on lines +111 to +113
Cs::set();
modm::delay_us(1);
Cs::reset();
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking wait would be nicer, see above...

@rleh rleh added the stale ♾ label Apr 9, 2023
@rleh rleh added this to the 2023q2 milestone Apr 9, 2023
@salkinium salkinium removed this from the 2023q2 milestone Jul 10, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Development

Successfully merging this pull request may close these issues.

3 participants