electro-smith / libDaisy

Hardware Library for the Daisy Audio Platform
https://www.electro-smith.com/daisy
MIT License
332 stars 143 forks source link

add MAX11300 ADC range & various small fixups #459

Closed TheSlowGrowth closed 2 years ago

TheSlowGrowth commented 2 years ago

This PR adds the 0 .. 2.5V ADC voltage range to the MAX11300 driver. This range is useful when connecting pots / sliders as it results in a much lower current through those components, compared to the full 10Vpp of the other ranges.

As this range is not available for the DAC outputs, the existing enum VoltageRange was split into two enums, enum DacVoltageRange and enum AdcVoltageRange.

Some other small fixups included in this PR:

TheSlowGrowth commented 2 years ago

@recursinging please take a look at the changes

recursinging commented 2 years ago

@TheSlowGrowth, Everything looks good to me. Splitting up the VoltageRange enum is the best way to safely make the 0..2V5 range available. The rest of the changes are sensible.

Thanks for the contribution!

TheSlowGrowth commented 2 years ago

@recursinging: I adjusted the tests. They expected the lowest bit of the range bits in FUNCPRM 8:10 to be set for digital read/write mode and the high impedance mode (that would equal to the default 0..10V range). All three of these modes don't actually use these bits, so I think it's safe to adjust the test expectations.

Am I correct?

recursinging commented 2 years ago

@TheSlowGrowth Yes, that is correct. There was an error in the logic of application of the range bits that has been corrected by your changes. The test expectations should be updated to reflect the corrected logic.

stephenhensley commented 2 years ago

@TheSlowGrowth @recursinging everything looks good to me!

I don't have any hardware with this device. So I'll defer to your testing, etc. I can merge whenever.

Assuming the changes to ringbuffer.h were just to get rid of some CMake build warnings for unused function arguments, or something?

TheSlowGrowth commented 2 years ago

@stephenhensley I tested it here on my tape looper board and it works just fine. I'm working on some more changes to support multiple MAX11300 from one driver, using one SPI bus with multiple software controlled NSS pins. That'll come soon, hopefully.

[...] the changes to ringbuffer.h [...]

Yes. I have a new macbook where the Xcode toolchain installs clang 13.0.0 by default. This results in warnings in the unit test build, which are elevated to errors. Could be worth to add a matrix build setting to the CI so that we catch these kind of errors earlier.

TheSlowGrowth commented 2 years ago

See #460

stephenhensley commented 2 years ago

@TheSlowGrowth sounds good to me! Looks like you're working on some fun stuff!

And yeah, that makes sense. I'd seen similar happen when using DaisySP with non-standard builds. Just wanted to clarify.