The 3.x change

[BenUniqcode]

Hi
Thanks so much for your library, which I've been using for quite a while.

I've just got around to updating from 2.x to 3.x and I see your reason for the major version bump is given as:

In versions < 3.0.0, there were several functions that returned a structure of type "xyzFloat". To be exact not the structure was returned but a pointer to this structure. Since the structures were created in the functions, the memory space where they were stored could have been overwritten. I have now changed this, but unfortunately, former sketches are not compatible anymore.

It's not a major problem to adapt my code, but I don't understand the reasoning.
Functions that returned a struct, returned a struct, by value. They didn't return a pointer to a struct inside the function.
Returning a struct by value is quite safe.
It's considered bad practice to do it for a large struct, because it's inefficient to copy a load of data. But xyzFloat is small.

So, the change wasn't necessary for that reason.

However, now that it's been done, there is a potential benefit. These functions could now use a bool return value to indicate success or failure. Currently the library doesn't really do any error checking of comms, and it would be great if it could be a bit more active in that regard. The underlying TwoWire::requestFrom() and SPIClass::transfer() will return 0, or less than the expected number of bytes, in the event of a comms error. You could check for that in functions like readRegister8() and readMultipleRegisters(), return false if there was an error, and pass that up the chain of functions, allowing the user to retry, or at least ignore the values in the xyzFloat if they should not be trusted.

Now, before you shrink from the amount of work this entails, here's the best news - I'll do it!

I'm intending to fork your library to adapt it for the ADXL366 (remarkable new device - power consumption in measure mode is of the order of 1uA, compared to >100uA for the ADXL345, and yet it's cheaper and has more features!). Because quite a lot has changed from the 345, I think is best done as a separate library, but I intend to keep the same API that this library uses wherever possible, so that it can be a drop-in replacement for users of the newer accelerometer. Anyway, as part of that work I thought it would be good to add the above error checking, and I will be happy to feed that part of it back to you as a Pull Request if you are interested.

All the best
Ben

[wollewald]

Hi @BenUniqcode , in another library I had the same approach like in ADXL345_WE version <3.x. I.e. was also using the xyzFloat struct and returned it the same way. There, someone else raised an issue because he was of the opinion, that returning a struct was not safe:
wollewald/ICM20948_WE#7 :
Btw, your myIMU.getMagValues() method approach may no be very commendable. It will return a pointer to memory that has been returned to the heap (free()) since it's a dynamic variable that gets out of scope with "return". It will probably work almost 100% but I would rather see myIMU.getMagValuesmagValue(&magValue).

So, I changed it without further discussion. But now I searched a bit and it seems to be OK how I did it before.

OK, but let's come back to the positive effect, which is the error check. I would appreciate if you implement it and thank you in advance!

[BenUniqcode]

Hi @wollewald
Great! I'm just working on this now. Couple of things I want to check with you.
I'm trying to avoid any more breaking changes to the API. With that in mind:

• For functions that return an enum type, such as getRange(), getOrientation(), getDataRate(), I think the best option is to add a value to the enums to indicate an error. e.g. ADXL345_RANGE_ERROR = -1. For safety, the setters like setRange() etc would explicitly check for that value and never send it to the ADXL. Does that sound ok to you?
• For functions that return a float, i.e. getPitch() and getRoll(), we either return 0.0, which is perhaps not ideal as that could in theory be a real reading, or to use some value that can't be real - as I think these figures are in degrees, a value of say 999.0 would be obviously wrong. What do you think?

Cheers
Ben

[wollewald]

Hi @BenUniqcode , no breaking changes sounds good. I like the solution for the enums (ADXL345_RANGE_ERROR = -1). I agree that returning a 0.0 as an indicator of an error is a bit misleading. The chances that exactly that exactly 0.0 is returned as a real value are low, but I would prefer an obviously wrong value.

Cheers, Wolfgang

[BenUniqcode]

Hi @wollewald - please see Pull Request #17 - any feedback is welcome.
Cheers
Ben