search

pasko-zh / brzo_i2c

Brzo I2C is a fast I2C Implementation written in Assembly for the esp8266
GNU General Public License v3.0
244 stars 47 forks source link

Make disabling interrupts optional #19

Closed flannelhead closed 7 years ago

flannelhead commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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