search

sparkfun / SparkFun_Bio_Sensor_Hub_Library

25 stars 33 forks source link

Extremely dangerous read methods #13

Open perezmeyer opened 3 years ago

perezmeyer commented 3 years ago

Hi! I'm afraid the code has extremely dangerous read methods: one can not differentiate a byte properly read from a wrong status byte.

Example: https://github.com/sparkfun/SparkFun_Bio_Sensor_Hub_Library/blob/master/src/SparkFun_Bio_Sensor_Hub_Library.cpp#L1263

In line 1277 if he read access fails the status byte is returned, but if the byte is read then the proper byte is returned. The user of the method does not has a way to differentiate between both. A proper solution would be to pass a uint8_t "statusByte" by reference and return the status byte in it, the user must then check it before using the result.

There are many methods with this kind of errors. If you would like me to present MRs please just say so.

Kinds regards, Lisandro.

edspark commented 3 years ago

You make a good point, there is no way as the user to differentiate between the statusByte and the returnByte. I'm not sure I understand your solution, but perhaps I can offer one? I could also make a different type specifically for status to help differentiate, what do you think?

perezmeyer commented 3 years ago

Well, I re did the code using also Maxim's mbed driver as reference and did this:

https://gitlab.com/mosimpa/esp32-firmware/-/blob/develop/mosimpa/src/drivers/max32664a.h https://gitlab.com/mosimpa/esp32-firmware/-/blob/develop/mosimpa/src/drivers/max32664a.cpp

I've also used multi-byte I²C calls whenever possible. This improved stability a lot. If the code helps you in anything please feel free to use it, even changing the license back to your original license too.

perezmeyer commented 3 years ago

Well, I re did the code using also Maxim's mbed driver as reference

Worth to mention: I 've also stripped down the code to the bits I needed, it could be easily get expanded again.

perezmeyer commented 3 years ago

Oh, and the code now uses the MAX32664's interruption in order to get the data instead of just polling. This was deemed necessary for our usage.

edspark commented 3 years ago

This is very helpful - looks much better than mine and uses much better coding practices! I'll use what's relevant to the issue you described, and perhaps improve the library over all if I have time.