techpaul / PS2KeyAdvanced

Arduino PS2 Keyboard FULL keyboard protocol support and full keys to integer coding
GNU Lesser General Public License v2.1
140 stars 27 forks source link

ESP32 garbage values #21

Closed dharmik768 closed 3 years ago

dharmik768 commented 3 years ago

I am using ESP32 with this library to read my PS2 keyboard. I am using a bi-directional logic level shifter based on MOSFET to convert 3.3v to 5v logic shift. The GPIO pin that I was using for the clock is 23 and for data 22. I uploaded the example code Simple_test. And when I type something with the keyboard it detects like when I press key 'a' then it prints 'a' on the serial monitor but many times when I do this little quickly by pressing different keys on the keyboard now ESP prints random garbage values on the serial monitor. For example, pressing 'g' on a keyboard and will show '2' on the serial monitor, and pressing the same key a second time shows 'g' properly.

Initially, I thought the logic converter I build had some issues so I tried the same code with Arduino UNO connected to it and it worked perfectly as expected so it's not a hardware issue.

Then I looked in the library code and get to know that it is using an interrupt on the clock pin and as I have been in a similar situation with ESP32 before while using interrupt I finally got to know why this thing is happening.

In Arduino ESP32 support there is some bug for an ESP32 interrupt routine. When we declare interrupt on falling edge in ESP32 with Arduino IDE with our ISR function. Then ideally the ISR function should get only triggered on the falling edge but when I checked with my logic analyzer it sometimes gets triggers on the rising edge also don't know the reason for that and also after such updates this thing is still not resolved. Have a look at this issue

So the simple solution for this problem can be implemented by adding one more if condition inside our ISR function like this.

void ps2interrupt(void)
 {
  if(digitalRead(irq_num_pin) == LOW)
  {

This will verify that the pin is in the LOW state or not by just adding this condition in .cpp file the ESP32 garbage problem was fixed and now I am getting perfect readings with my ESP32.

I defined a new variable as irq_num_pin which is equal to the clock pin called in begin function.

So I suggest updating this little logic so that it works better with an ESP32. Also, I have not tested it with Arduino UNO after making this change but I am sure it will not make any difference for other boards.

techpaul commented 3 years ago

I will look at incorporating this into this and associated libraries in a few days (other important things first)., more likely as conditional compile for simplicity of support.

Suspect the ESP32 ISR edges issue is a bug in that core as I doubt a micro does not have one edge OR both edges capability.

Thanks for the heads up

n6il commented 3 years ago

I'm working with ESP32 as well and discovered the same thing just last night.

It might be easier to use this solution. Since you want the FALLING edge, if you get in the ISR and the current level is HIGH then reject it:

void ps2interrupt( void )
{
#ifdef ESP32
if(digitalRead(PS2_IrqPin))
   return;
#endif
dharmik768 commented 3 years ago

Yes, michaels logic will be more efficiente as it targets ESP32 particularly, to fix this interrupt issue.

techpaul commented 3 years ago

That is a similar technique I would use and needs to be looked at for a few libraries with user supplied ESP32 support.

Generally prefer the early return on data to ignore or error conditions, less indenting and having to match blocks makes it easier to maintain and more readable

techpaul commented 3 years ago

As ESP32 is not one of the listed support, I perhaps need more testing from yourselves as other architecture based defines are set according to board support. Especially when writing to LEDs on keyboard due to interrupts being triggered inside an ISR when sending.

This suggests you are ignoring the compilation WARNING (always correct ALL warnings)

#warning Library is NOT supported on this board Use at your OWN risk
techpaul commented 3 years ago

When I get time I will put up a test version for ESP32 when I know what some of the board support parameters like ARDUINOARCH????? in order to incorporate it.

This test branch will need testing by users with the boards BEFORE public release.

techpaul commented 3 years ago

I am looking at this issue and I personally do not like this workaround as this is a SILICON issue with the mcu and any spurious interrupts should be handled by arduino-esp32. This SILICON issue is detailed in their own errata document

https://www.espressif.com/sites/default/files/documentation/eco_and_workarounds_for_bugs_in_esp32_en.pdf "3.14. The ESP32 GPIO peripheral may not trigger interrupts correctly. Details: The ESP32 GPIO peripheral may not trigger interrupts correctly if multiple GPIO pads are configured with edge-triggered interrupts."

The workarounds should have been in the interrupt dispatcher for arduino-esp32

I am working on a better solution for the library

techpaul commented 3 years ago

Pre-release candidate V1.09 is available for testing, tell me of any issues https://github.com/techpaul/PS2KeyAdvanced/releases/tag/V1.0.9 Unzip into the libraries folder to update if you do not have fork pull ability

Some documentation has been updated as well

techpaul commented 3 years ago

This has been realeased as V1.0.9 so closing issue