Return types of differential ADC-count functions are misleading

[BenjaminPelletier]

When the ADS1256 takes a differential measurement, it returns a signed 24-bit integer -- this value is conceptually positive when one of the differential inputs has a higher voltage than the other, and this value is conceptually negative when one of the differential inputs has a lower voltage than the other. Negative values are expressed via the two's complement method, and therefore any value with the 24th bit set is conceptually negative. Furthermore, the function signature of the differential functions (like cycleDifferential) indicate that they return a signed value (long is equivalent to a 32-bit signed number on most platforms). Therefore, I would expect a negative differential reading returned from cycleDifferential to be negative. For instance, if I measured -1 ADC counts, I would expect

Serial.print(adc.cycleDifferential());

...to print -1 to the serial port. However, since the 24 bit value from the ADS1256 is returned directly from cycleDifferential, the value is not interpreted as negative. Instead, small negative values will appear as positive values around 16 million (0xFFFFFF). In the above example, I would print 16777215 to the serial port even though the ADS1256 reported that it read -1 counts. This is very surprising and undesirable.

One approach could be to implicitly "warn" users of this surprising behavior by returning an unsigned long or uint32_t from cycleDifferential -- this would at least communicate that there is no chance of observing a "negative value" from this function, so the user would know to perform the two's complement transformation when expecting potentially-negative values. But, a better approach would be to return the actual number the 24-bit hex value from the ADS1256 represents. This can be done by changing this line to include this logic, and then remove that logic from convertToVoltage since the input value will already be negative.

I can propose a PR if that suggestion isn't clear.

[CuriousScientist0]

Hi! I think I understand it. The initial reason why I made the functions (not only the cycleDifferential()!) like this is because I thought that some more adventurous and curious users could write their own functions, macros and whatnot to convert the "absolute raw" reading of the ADC to whatever format they want.

Since the datasheet in Table 16 details all the possible data formats, I thought that the user could use it as a reference and format it to their own purpose as they want. And then I provide an additional function that takes care of both the conversion to a "real negative number" and to a voltage, based on the reference voltage and the PGA settings.

So, honestly, I would not change the code, but I would rather write a few extra lines in the documentation to warn the user about the behaviour. For me who spent a few years studying the datasheet, I got used to the return values and the format, so I don't feel that the values are misleading. However, for someone new to the chip, code...etc., it might feel like.

Let me know what you think!

[BenjaminPelletier]

I am definitely interested in the raw ADC counts (and actually have no use for voltages as I only care about readings relative to themselves in order to detect something akin to a button push), but the results from, e.g., cycleDifferential don't behave like raw ADC counts. For instance, in the test code for #12, I check the ADC-count output with value2 > THRESHOLD || value3 > THRESHOLD || value4 > THRESHOLD. This code works fine when the values are interpreted as ADC counts (i.e., with negative 24-bit values corresponding with negative numbers), but it breaks if I use the current output of cycleDifferential because -1 is less than the THRESHOLD (~3 V) value, but 16777215 is not less than the THRESHOLD value.

I would suggest that there isn't really a reason to express a negative number as a positive number. If someone is interested in the actual bits of the 24-bit return value from the ADC, they can still get that after my proposed changed by masking the lower 24 bits (this is the magic of two's complement). With my change, changeDifferential would return 0xFFFFFFFF instead of 0x00FFFFFF when the 24-bit value was -1. The only difference is that C++ understands 0xFFFFFFFF as "-1" whereas C++ understands 0x00FFFFFF as "16777215" even though the ADC intended 0xFFFFFF to communicate "-1". So, I recommend that the current function is not accurately relaying the information the ADC intends to convey.

If you don't like the change in #13, I would recommend, at a minimum, to change the return type of the functions from a long (which is a signed value, meaning that the highest bit is intended to indicate the sign of the number) to an unsigned long (uint32_t) because the latter clearly communicates that no bit is used to indicate the sign of the value. The use of the former communicates the wrong thing to the user because it says "this number could be negative, but the value 16777215 is not negative" even though the ADC intended a negative value when communicating those bits.

[CuriousScientist0]

I think your reasoning makes sense! I merged your suggestion, thank you very much! I will soon update the documentation and then refresh the whole library to "v1.3" which will also appear as a new version in the official Arduino Library Manager.