[BUG] Encoder behavior change after “🚸 Encoder improvements (#26501)”

[vovodroid]

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

BTT TFT35 SPI screen encoder skips steps on BTT SKR-3, it means that it should be turned by two detentions for single screen action.

Related to Add support for SPI TFT displays and touch screen for STM32H7 MCUs #25784

Bug Timeline

No response

Expected behavior

No response

Actual behavior

No response

Steps to Reproduce

No response

Version of Marlin Firmware

bugfix-2.1.x Bump distribution date (2023-12-31)

Printer model

Custom

Electronics

BTT SKR-3 board

LCD/Controller

BTT TST35 SPI screen

Other add-ons

No response

Bed Leveling

None

Your Slicer

None

Host Software

None

Don't forget to include

• [X] A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

https://github.com/MarlinFirmware/Marlin/assets/8691781/f07d6e05-eed5-4bc6-90b5-b979176c9764

Config.zip

[thisiskeithb]

Encoder behavior changes are related to https://github.com/MarlinFirmware/Marlin/pull/26501.

[vovodroid]

Will decreasing ENCODER_PULSES_PER_STEP help?

[classicrocker883]

Will decreasing ENCODER_PULSES_PER_STEP help?

this was my first thought. maybe double (increase) rather than decrease?

or how about

//
// Use this option to override the number of step signals required to
// move between next/prev menu items.
//
#define ENCODER_STEPS_PER_MENU_ITEM 1

[vovodroid]

@classicrocker883

maybe double (increase) rather than decrease?

Decrease. I set it to one, and got 4 steps per detent, so ENCODER_PULSES_PER_STEP 4 gave perfect one step.

ENCODER_STEPS_PER_MENU_ITEM 1

It's one by default if not defined.

@thisiskeithb According to my measurement it's four pulses per step in BTT_TFT35_SPI_V1_0. There only two printers with this display in configuration - B1 SE and SE Plus. Would like me to create PR or you just change it by yourself?

[thisiskeithb]

Would like me to create PR or you just change it by yourself?

Let's wait for a proper fix before a workaround is submitted. I've pinged @dbuezas again in hopes they can help track down the fix.

[dbuezas]

Hi! I really can't reproduce this with my encoder, may I suggest an experiment for you to do with your setup? Add a serial log with the contents of the encoderDiff variable. It seems to be losing steps somehow.

Also make sure ENCODER_STEPS_PER_MENU_ITEM exactly matches the haptic "click" of the encoder. To do that, test that when you scroll in one direction all and every dent results in a scroll. If this constant is set to 4 but should actually be 3, this exact behavior would occur

[classicrocker883]

wasn't ENCODER_STEPS_PER_MENU_ITEM 1 commented out in your Configs?

[dbuezas]

Mine is 2

[vovodroid]

@dbuezas

Add a serial log with the contents of the encoderDiff variable.

Can you provide me with piece of code please?

To do that, test that when you scroll in one direction all and every dent results in a scroll.

If rotation is relative slow it works exactly one step per click with ENCODER_PULSES_PER_STEP 4 within several revolutions back and forth. But it skips steps if rotation is getting faster.

@classicrocker883

wasn't ENCODER_STEPS_PER_MENU_ITEM 1 commented out in your Configs?

It was, but there is code in Marlin\src\inc\Conditionals_LCD.h:

#ifndef STD_ENCODER_STEPS_PER_MENU_ITEM
  #define STD_ENCODER_STEPS_PER_MENU_ITEM 1
#endif

#ifndef ENCODER_STEPS_PER_MENU_ITEM
  #define ENCODER_STEPS_PER_MENU_ITEM STD_ENCODER_STEPS_PER_MENU_ITEM
#endif

so final value is one.

[dbuezas]

As an experiment, how does it work if you set ENCODER_PULSES_PER_STEP to 2 or 3?

[vovodroid]

With ENCODER_PULSES_PER_STEP 2 it jumps 2 steps per detent, as expected.

[vovodroid]

With ENCODER_PULSES_PER_STEP 2 it jumps 2 steps per detent, as expected, skipping when rotated fast.

[dbuezas]

And reversing direction still gives asymmetric results, I assume.

Does 3 also result in weird behaviour?

Scrolling fast should be a different issue, namely that the mcu doesn't look at the pins fast enough so it misses some. The usual solution is to read them via pin change interrupts instead of a timer, I'm not sure why Marlin stopped doing that, surely for good reasons.

Could you also try reverting this line? It is intended to guard a bit more against noise (and works well in my setup), but still worth a try. https://github.com/MarlinFirmware/Marlin/pull/26501/files#diff-3c8b6ef45c61c9cff05542db2f1b61eb8926eaf3e33f9b4d952323fdce5bddf8R1423

[vovodroid]

And reversing direction still gives asymmetric results, I assume.

Do you mean skipping? Yes.

Does 3 also result in weird behaviour?

I didn't try. If 2 pulses results in 4 steps, 2 in 2, and 4 in 1, 3 will be just not synchronized with detends, I believe.

Could you also try reverting this line?

I.e. remove if (!ignore)? I tried, it skips more steps.

[dbuezas]

With asymmetric I mean if reversing moves a different amount than keeping direction.

3 will be just not synchronized with detends

Right, 3 wouldn't be in sych with the dents. But since you observe weird behaviour with 4, maybe it is 4 the one that gets out of synch. 🤷 I'm shooting in the dark.

Do you know the model of the encoder? I'd like to take a look at its datasheet

[thisiskeithb]

Do you know the model of the encoder? I'd like to take a look at its datasheet

This happens on multiple displays/encoders of various types, manufactured at different times by various OEMs, and fall in the "generic"/unbranded category for what I have here.

[vovodroid]

@dbuezas

With asymmetric I mean if reversing moves a different amount than keeping direction.

No, direction doesn't matter.

since you observe weird behaviour with 4,

Why weird? It works very nice with 4, just skipping on fast rotating.

Do you know the model of the encoder?

Screen is BTT TFT35 SPI v1.0.

[dbuezas]

Then the only hypothesis I have is that reversing direction somehow results in a very quick pair of steps and the firmware is missing one of them. The new logic is rather simple and I fail to see the bug, plus it works on my machine™️ (which has 2 steps per dent).

[dbuezas]

Why weird? It works very nice with 4, just skipping on fast rotating.

Got it. I meant the lost movement when reversing direction, but if it never misses a step in a long list (e.g files) when moving in the same direction, then my suggestion wont help. It may fix the reversing case, but it will start jumping 2 lines at once every 3rd dent.

I'm running out of ideas

[thisiskeithb]

I'm running out of ideas

It'd probably be a good idea to revert the changes before the next release or we're going to have a lot more users experiencing this new behavior/bug.

[vovodroid]

By a way, why it's #define ENCODER_PULSES_PER_STEP 5 in Marlin config for B1 SE Plus with this screen https://github.com/MarlinFirmware/Configurations/blob/import-2.1.x/config/examples/BIQU/B1%20SE%20Plus/Configuration.h#L2690C20?

BTT uses

//#define ENCODER_PULSES_PER_STEP 4
#define ENCODER_STEPS_PER_MENU_ITEM 2

https://github.com/bigtreetech/Marlin/blob/SE-Plus-IDEX-2.1.x/Marlin/Configuration.h#L2501 so it gives 2 pulses per step (and faster value change) here https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.1.x/Marlin/src/inc/Conditionals_LCD.h#L973C42-L973C42

It corresponds with my finding 4 pulses per step.

[thisiskeithb]

why it's #define ENCODER_PULSES_PER_STEP 5 in Marlin config for B1 SE Plus

Because that's what works on all the TFT35 SPI displays I have over here (tested on a BIQU B1 SE & SE Plus machines + a standalone TFT35 SPI display). We often fix/adjust configs from OEM defaults since they are often wrong or not optimally configured.

You don't have to use config defaults, so feel free to update your firmware as you see fit.

[dbuezas]

It'd probably be a good idea to revert

Your repo, your rules :)

I'll try to get hold of a 4 step per dent encoder so I can debug this before opening a new PR.

[vovodroid]

Because that's what works on all the TFT35 SPI displays I have over here (tested on a BIQU B1 SE & SE Plus machines + a standalone TFT35 SPI display). We often fix/adjust configs from OEM defaults since they are often wrong or not optimally configured.

May be after #26501 it works correctly?

[thisiskeithb]

May be after https://github.com/MarlinFirmware/Marlin/pull/26501 it works correctly?

No, that’s the source of these encoder issues. It needs a follow-up PR with a fix or to be reverted until more testing can be done on a wider range of hardware.

[thinkyhead]

Just to be on the safer side I've reverted all the changes from #26501. To reiterate, I did spend a bunch of time on the encoder logic using a few displays way back in history, so the logic that I arrived at should be largely retained. The issue(s) that are meant to be addressed by #26501 should be explored further, but with a working group using a few different displays and with lots of logging. I'm hopeful that the final solution will only consist of adding a line or two, but we can also do some research on other encoder implementations to see how often they poll and how they deal with missed encoder states, very fast spinning, and flaky encoders that miss a detente until you click the encoder button and unwittingly move the encoder.

[vovodroid]

I've reverted all the changes from #26501.

I built with this revert but with #define ENCODER_PULSES_PER_STEP 4 remained. It still makes exact one step per detent, but skips more when rotating a bit faster. From my POV with #26501 it was better.

[thinkyhead]

We'll start by focusing on that data collection. Some say there are new issues with the updated code while some say there are issues with the old code. Both may be true. There is likely some artifact(s) that the previous code deals with, and some other artifact addressed by the new code.

We can and should perform a thorough analysis of the raw incoming data from the encoder, logging the encoder states and the current millis() and go from there. We can emit data in CSV so it's easy to graph in Google Sheets. This will also show us any flaws in the encoders themselves. I'll go ahead and add that logging to the code and then we can collect data on…

• CW slow, fast, and very fast rotation.
• CCW slow, fast, and very fast rotation.
• Single detente back and forth (frequently reversing direction).
• Haphazard rapid reversing direction.
• Rotate, then click. (note any missed menu items or edited values).
• Click and hold.
• "Double-click."
• etc.

[dbuezas]

@thinkyhead: If nobody took this by then, I'll give this another shot when I'm back from vacations. Is there any specific reason not to use pin change interrupts to keep encoder state?

I made a quick experiment a month ago and it also lost steps, but I couldn't find any other piece of code touching the encoder variables.

[ellensp]

older 8 bit board pin change is not available on all IO pins.

[thinkyhead]

@dbuezas — When interrupts are available, an interrupt-based method may be used, and I'm all for that. Since there's more than one encoder implementation, including a touch screen simulated encoder, those would also need to be accounted for in any new implementation.

Since we can't entirely eliminate it, I propose that we first aim to fix up the polling-based method and have it poll more frequently so encoder events aren't missed. The first step is to make a MarlinUI::update_encoder method based on the encoder logic at the top of ui.update. (Other buttons can still be handled in ui.update.) The separate ui.update_encoder method can then be polled more frequently from within idle() and then it can also serve as the interrupt callback for the encoder CLK pin when using the interrupt-based encoder.

One possible source of missed steps is that we define the EN1/EN2 pins without complete clarity on which is CLK and which is DATA. It might be that some encoder anomalies come from these pin definitions being reversed in the pins file, and possibly our encoder implementation not quite getting the interpretation of the CLK/DATA states quite right. So I propose in our testing and experiments that we also try swapping the EN1/EN2 pins in conjunction with reversing the encoder direction to see how that affects behavior.

[dbuezas]

@thinkyhead I made some experiments with pin change interrupts and it made the encoder go completely crazy. This indicates a debouncing issue. With that in mind I added a debouncing logic and WHAT A DIFFERENCE, even at fast rotations.

I'm now pretty sure all issues are related to bouncing, I'll work on this some more on the coming weeks and make a new PR.

[thinkyhead]

This example considers a 5ms debounce sufficient to filter noise.

[dbuezas]

I need to do more tests, but found that waiting for a pin to become stable for as low as 0.1ms seems to be enough. There seems to be a sweet spot between stability at low and fast speeds. I want to add a mechanism to detect skipped steps in both directions

[thisiskeithb]

Just to close this out / link things together, encoder changes were reverted in 0f43ac7.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.