adafruit / Adafruit_MCP3008

MCP3008 8-Channel 10-Bit ADC
MIT License
24 stars 14 forks source link

Software SPI Version - Missing last bit #5

Closed yngndrw closed 3 years ago

yngndrw commented 3 years ago

The code which reads a value from the ADC via a software SPI port seems to be missing a bit. The maximum value reported is 512. Only 16 bits (Including the dummy bits used to read back the result) are sent to the ADC, but the datasheet calls for one more - The hardware SPI version sends a total of 24 bits. https://github.com/adafruit/Adafruit_MCP3008/blob/master/Adafruit_MCP3008.cpp#L150-L167

Here's a version which I used to test the theory that it is just missing the last bit - I hacked in the last bit after the loop:

    uint16_t outBuffer, inBuffer = 0;

    digitalWrite(cs, LOW);

    // 5 command bits + 1 null bit + 10 data bits = 16 bits
    outBuffer = command << 8;
    for (int c = 0; c < 16; c++) {
      digitalWrite(mosi, (outBuffer >> (15 - c)) & 0x01);
      digitalWrite(sck, HIGH);
      digitalWrite(sck, LOW);
      inBuffer <<= 1;
      if (digitalRead(miso))
        inBuffer |= 0x01;
    }

    digitalWrite(mosi, 0x00);
    digitalWrite(sck, HIGH);
    digitalWrite(sck, LOW);

    inBuffer <<= 1;
    if (digitalRead(miso))
      inBuffer |= 0x01;

    digitalWrite(cs, HIGH);

    return inBuffer & 0x3FF;
caternuson commented 3 years ago

Do you get the same behavior if you use the library example invoked with software SPI? https://github.com/adafruit/Adafruit_MCP3008/tree/master/examples/simpletest

Quick test with an Itsy M0 works OK. Channel 0 connected to VCC gets expected 1023. All other channels are floating, so ignore values.

Screenshot from 2021-06-18 13-29-07

yngndrw commented 3 years ago

Just tried that sample with the following change and saw the same result with all channels set to the same as the reference (3.3V in my case):

adc.begin(25, 26, 27, 33);
22:29:29.694 -> 511 511 511 511 511 511 511 511 [0]
22:29:30.645 -> 511 511 511 511 511 511 511 511 [1]
22:29:31.653 -> 511 511 511 511 511 511 511 511 [2]
22:29:32.665 -> 511 511 511 511 511 511 511 511 [3]

I then changed it to use a custom SPIclass instance (Still using the same pins which do not support hardware SPI) and this resolved the issue as it's now using the "Hardware SPI" code path:

22:35:34.182 -> 1023    1023    1023    1023    1023    1023    1023    1023    [0]
22:35:35.167 -> 1023    1023    1023    1023    1023    1023    1023    1023    [1]
22:35:36.146 -> 1023    1023    1023    1023    1023    1023    1023    1023    [2]
22:35:37.150 -> 1023    1023    1023    1023    1023    1023    1023    1023    [3]
#include <SPI.h>
#include <Adafruit_MCP3008.h>

SPIClass spi(HSPI);
Adafruit_MCP3008 adc;

int count = 0;

void setup() {
  Serial.begin(9600);
  while (!Serial);

  Serial.println("MCP3008 simple test.");

  spi.begin(25, 27, 26, -1);
  adc.begin(33, &spi);

  // Software SPI (specify all, use any available digital)
  // (sck, mosi, miso, cs);
  //adc.begin(25, 26, 27, 33);
}

void loop() {
  for (int chan=0; chan<8; chan++) {
    Serial.print(adc.readADC(chan)); Serial.print("\t");
  }

  Serial.print("["); Serial.print(count); Serial.println("]");
  count++;

  delay(1000);
}
yngndrw commented 3 years ago

Here's the timing diagram from the datasheet, showing how 17 clock pulses are required to retrieve the full result: image

The rising edge of that 17th clock pulse is responsible for clocking out that last (least significant) bit.

caternuson commented 3 years ago

OK. Looks like this might be ESP32 (any others?) specific. Just tried with a Feather ESP32 and seeing that behavior.

Screenshot from 2021-06-18 15-23-16

yngndrw commented 3 years ago

From the timing diagrams within the datasheet and the implementation within the soft serial portion of the library, I can't understand how different microprocessors would change the outcome - We're not using a hardware peripheral here, just bitbanging the required data so any microprocessor should behave the same. The only thing that I can think of is that the timing is slightly different, but I can't see how that would happen.

I've only tried it with the ESP32, I'm using a test PCB that I made so I can't easily switch it out for something else I'm afraid.

caternuson commented 3 years ago

Also not sure why different MCU works. But current library code is a bit...dated. There's a good chance the update for #6 will fix this also.

caternuson commented 3 years ago

@yngndrw Please try the 1.3.0 release: https://github.com/adafruit/Adafruit_MCP3008/releases/tag/1.3.0

yngndrw commented 3 years ago

@caternuson I've just tried both constructors in the new version.

The constructor which takes a pointer to an instance of SPIClass works perfectly with the full range of results:

SPIClass spi(HSPI);

// ...

spi.begin(SpiSck, SpiMiso, SpiMosi, -1);
adc.begin(SpiCsAdc, &spi);

The constructor which takes individual pin numbers can no-longer speak to the ADC, every channel just returns 0:

_adc.begin(SpiSck, SpiMosi, SpiMiso, SpiCsAdc);
caternuson commented 3 years ago

Hmm. The sketch below is working fine on a Feather ESP32. Can you run the same sketch and if possible use the same pins?

#include <Adafruit_MCP3008.h>

Adafruit_MCP3008 adc;

int count = 0;

void setup() {
  Serial.begin(9600);
  while (!Serial);

  Serial.println("MCP3008 simple test.");

  // Software SPI (specify all, use any available digital)
  if (!adc.begin(13, 27, 12, 33)) {
    Serial.println("Error.");
    while (1) delay(10);
  }
}

void loop() {
  for (int chan=0; chan<8; chan++) {
    Serial.print(adc.readADC(chan)); Serial.print("\t");
  }
  Serial.print("["); Serial.print(count); Serial.println("]");
  count++;
  delay(1000);
}
yngndrw commented 3 years ago

Sorry it does work fine, I had a left in a call to the begin method of SPIClass and this was conflicting. Both constructors work correctly now.

caternuson commented 3 years ago

OK, cool. And original issue is fixed also?

yngndrw commented 3 years ago

Yes, the full range is output when using either of the constructors. :)

caternuson commented 3 years ago

OK. Thanks. Closing issue then.