Callback function data pointer is not a pointer to the ESP32Encoder instance

[matthewbeckler]

Hello, thanks for this great library. Hard to find an encoder library that uses interrupts or the PCNT hardware peripheral, and also supports registering a callback function.

However, there's a bug here (or at least a documentation bug) regarding the pointer passed to the callback function.

Per the docs:
https://github.com/madhephaestus/ESP32Encoder/blob/master/src/ESP32Encoder.h#L32
"@param enc_isr_cb callback executed on every encoder ISR, gets a pointer to the ESP32Encoder instance as an argument"

Per the example:
https://github.com/madhephaestus/ESP32Encoder/blob/master/examples/Encoder_interrupt_display/Encoder_interrupt_display.ino#L8
The ISR here casts the void* v to ESP32Encoder*, but this is always a null pointer.

The instantiation of the ESP32Encoder class here:
https://github.com/madhephaestus/ESP32Encoder/blob/master/examples/Encoder_interrupt_display/Encoder_interrupt_display.ino#L18
ESP32Encoder encoder(true, enc_cb);
does not provide a third parameter here, hence the null pointer passed into the callback function.

We could fix this by changing this line:
https://github.com/madhephaestus/ESP32Encoder/blob/master/src/ESP32Encoder.cpp#L88
esp32enc->_enc_isr_cb(esp32enc->_enc_isr_cb_data);
to something like
esp32enc->_enc_isr_cb(esp32enc->_enc_isr_cb_data ? esp32enc->_enc_isr_cb_data : esp32enc);
where it could return the constructor-set _enc_isr_cb_data if one was provided at construction time, otherwise it would return a pointer to the ESP32Encoder instance.

I can prepare a PR for this if you like. Thanks!

[guidoschreuder]

maybe better to do it in the constructor?
like: if (enc_isr_cb_data == nullptr) { _enc_isr_cb_data = this; }

[ztjona]

maybe better to do it in the constructor? like: if (enc_isr_cb_data == nullptr) { _enc_isr_cb_data = this; }

In the case somebody does not know where. It must be added between these lines:

ESP32Encoder/src/ESP32Encoder.cpp

Lines 45 to 46 in 9979722

	{
	}