Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extremely dangerous read methods #13

Open
perezmeyer opened this issue Nov 25, 2020 · 5 comments
Open

Extremely dangerous read methods #13

perezmeyer opened this issue Nov 25, 2020 · 5 comments

Comments

@perezmeyer
Copy link

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
Copy link
Collaborator

edspark commented Dec 15, 2020

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

edspark commented Dec 15, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants