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

Make disabling interrupts optional #19

Closed
flannelhead opened this issue Mar 20, 2017 · 4 comments · Fixed by #20
Closed

Make disabling interrupts optional #19

flannelhead opened this issue Mar 20, 2017 · 4 comments · Fixed by #20

Comments

@flannelhead
Copy link
Contributor

A huge thank you for this library. It's easily the best I2C driver one can have on ESP8266, in terms of speed and usability.

However, I recently had some problems using this library, sampling an IMU via I2C at 1 kHz and doing continuous WebSocket communication and PWM realized by the HW timer simultaneously. When the interrupts were disabled during I2C transactions, there were frequent stalls / resets, probably caused by some clashes due to missed interrupts. I then commented out the lines where the interrupts were disabled, and stalls became less frequent. (There are still some, but they aren't probably related to brzo_i2c.)

Therefore it would be great to have a configuration option in brzo_i2c.h where the user could opt out of disabling the interrupts. This should be a simple addition, and the default behaviour should be left unchanged.

I would be able to supply the patch as a pull request if desired.

@pasko-zh
Copy link
Owner

Glad to hear that brzo_i2c is useful for you 😄

Yes, this sounds reasonable. Actually, I thought of such an option, too. But due to lack of time I did not change it yet. I agree that having this option "globally" is enough, i.e. we don't need to pass it to each brzo_i2c_write / read as optional parameters.

Well, since you asked that nicely: If you are able to do a pull request, it is very welcome!

@flannelhead
Copy link
Contributor Author

@pasko-zh I'll open a PR as soon as I have time to finish it.

Do you think a #define statement in the header would suffice? Or should it still be allowed to set that at runtime via e.g. a function brzo_i2c_set_disable_interrupts?

@pasko-zh
Copy link
Owner

pasko-zh commented Mar 21, 2017

I think it is enough with a #define. Otherwise, one should think of enable/disable the interrupts for each write/read or to move it on the level of an i2c transaction, i.e. pass it through an additional paramter.
But I think this fine grained enbaling/disabling is not really a need, right?

@flannelhead
Copy link
Contributor Author

@pasko-zh At least not for my use case. I suppose a #define is enough for now.

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

Successfully merging a pull request may close this issue.

2 participants