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

MPP v2+, Slim Pen 2, Surface Pro 9 improvements #156

Merged

Conversation

iwanders
Copy link
Contributor

Hi, this PR should address the long standing Slim Pen 2 button glitching from #102, #99 and quo#5.

Unfortunately I spent a lot of time at first trying to fix this by just using the current data and improving the detection / threshold. It all became easier when I took a step back and took a fresh look at all the data we get from the hardware with the MPP v2+ pens, without assuming DftButton actually holds button data. My analysis can be found at iwanders/analysis-ithc-iptsd, that page should probably be linked in one of the reverse engineering issues, but I don't know which one would be preferred. Fyi @quo, @qzed, @StollD, huge shoutout to the work you have been doing thus far.

In this PR:

  • Barrel button detection using the first 0x0a window, using row 4 and 5. Implementation is a bit hacky as the current data isn't grouped, but I saw @StollD modifying the parser handling this week, so filing this PR may help shape the direction to work towards.
  • Contact detection using the (previously called) Position2 frame, row 2 and 3.
  • Add config flag to specify Microsoft Pen Protocol 2.0+.
  • Add a preset for the Surface Pro 9.
  • Add some logging around the configuration loading, I derped around a bit only to discover my config in /etc/iptsd/iptsd.conf wasn't being loaded and I ended up with the defaults from the code, oops, there should now be a warning if no configuration is loaded. Should've probably been a different PR, but I ran into it while working on this.
  • Logic is based on recordings (both Windows & Linux) on a SP9 with Slim Pen 2, Metapen M2, Pen from AliExpress, and against data provided by @NP-chaonay in one of the issues.
  • I tried to make the commits in the appropriate style, but it may be easier to just squash them, I don't think the individual commits pass the linter.

Known limitations / deficiencies:

  • The button detection a approach means that I lose the eraser functionality for the Pen from AliExpress (eraser is done with a button, which also triggers the 0x0a row 4 & 5 switch), but I don't know how to fix that without using multiple frames instead of a single frame & window, we'd have to detect the alternating pattern.
  • This PR does not address the situation where the position interpolation fails here and results in the pen being lifted.
  • Have not done extensive prolonged testing of this approach. I cannot test on different hardware. At some point this repo should have integration tests running against data dumps.

Filing this as a draft PR for discussion, current window naming is obviously a stop gap solution, hopefully this PR also gives others a chance to help test, I'll have limited time to test, but will (obviously) make time to incorporate feedback and answer questions.

src/core/generic/config.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/linux/config-loader.hpp Outdated Show resolved Hide resolved
src/core/linux/config-loader.hpp Outdated Show resolved Hide resolved
src/core/linux/config-loader.hpp Outdated Show resolved Hide resolved
src/ipts/parser.hpp Outdated Show resolved Hide resolved
src/ipts/protocol/dft.hpp Outdated Show resolved Hide resolved
Comment on lines +235 to +237
* Determines the current button state from the 0x0a frame, it can
* only be used for MPP v2 pens. The eraser is still obtained from the
* phase using the button frame.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your notes I gather that this can't be used with MPP v1 pens because it contains garbage?

Is there any way to detect if the data inside is usable, and adjust the behaviour of the DFT parser accordingly? Dropping the need for a config option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to detect if the data inside is usable, and adjust the behaviour of the DFT parser accordingly? [...] I don't really like the idea of having to make the model / type of the pen a config option

Hmm, yeah, I'm not a fan of it either, because it means you can't use a mpp v1 pen without changing the config.

It may actually be possible... if a v1 pen is present, the magnitudes for the 0x0a frame are very small (100s). So what we could do I guess is check if the signal in the 0x0a frame is above a threshold (50000); if so, it trumps the button detection from the button frame.

Let me do a bit more exploration around this to see if I can make it work without explicit configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think this is possible, we can use a std::optional<bool> to specify whether the v2 signal has been detected.

I do think it is a good idea to allow different minimum magnitudes for the v2 button vs the v1 button detection though, the scales are completely different;

SP, without touching button.
mag4 and 5: 3668922    69
Button mag x and y: 4321    25085
mag4 and 5: 3583622    54
Button mag x and y: 4930    24469
mag4 and 5: 3627867    34
Button mag x and y: 5128    25154

SP, 5mm, going into button
Button mag x and y: 4    16
mag4 and 5: 333657    2
Button mag x and y: 39464    33930
mag4 and 5: 26    333492
Button mag x and y: 41810    34442

SP, touching, going into button
Button mag x and y: 5    32
mag4 and 5: 3980021    61
Button mag x and y: 308425    521441
mag4 and 5: 114    3756423

m1, 5mm, going into button
Button mag x and y: 5    0
mag4 and 5: 20    15
Button mag x and y: 26065    26065
mag4 and 5: 8    10
Button mag x and y: 23953    22730

m1, 5mm
Button mag x and y: 17945    16769
mag4 and 5: 70    155
Button mag x and y: 19109    16840
mag4 and 5: 127    37
Button mag x and y: 19825    16160
mag4 and 5: 43    19

m1, touching
Button mag x and y: 434394    273589
mag4 and 5: 42    31
Button mag x and y: 558101    235801
mag4 and 5: 29    74

m2, touching
Button mag x and y: 275221    193257
mag4 and 5: 8502    2500942
Button mag x and y: 276692    186628
mag4 and 5: 8338    2490670

If we use the same threshold, I can only detect my m1 pen's button when it is super close to the screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is now implemented under 772fbaf.

I'll need to give it a bit more thorough testing, but from the data I see no reason why this shouldn't work. And brief tests with all four pens show that this works, it does allow me to just switch between the SP, M1, M2 and CN pens without any problems in regards to button detection.

Great idea to just detect this, I did add different magnitude detection for the v2 signals to the configuration, because they do have a very different amplitude from the v1 button signal, but that's still a lot better than having to set a boolean.

@StollD
Copy link
Member

StollD commented Feb 26, 2024

First of all, thank you! This seems like it was a lot of work.

I added some comments and suggestions (and nitpicks). Unfortunately I don't have a device with a DFT based stylus, so I can't test it.

I don't really like the idea of having to make the model / type of the pen a config option, but I don't know enough about the hardware to suggest any alternative.

  • I tried to make the commits in the appropriate style, but it may be easier to just squash them, I don't think the individual commits pass the linter.

As long as the final commit passes, I don't really care. The linting is more me being obsessive anyways.

The button detection a approach means that I lose the eraser functionality for the Pen from AliExpress (eraser is done with a button, which also triggers the 0x0a row 4 & 5 switch), but I don't know how to fix that without using multiple frames instead of a single frame & window, we'd have to detect the alternating pattern.

If adding a temporal component to the DFT parser improves the situation, I'd be all for it.

@iwanders
Copy link
Contributor Author

but I don't know how to fix that without using multiple frames instead of a single frame & window, we'd have to detect the alternating pattern.

If adding a temporal component to the DFT parser improves the situation, I'd be all for it.

Problem is that in the CN pen, the 4 to 5 switch still happens when the eraser button is pressed;

Image for depressing eraser, then make screen contact. :

2024_02_25_linux_cn_hover_erase_touch

For hover, press barrel, touch screen, release barrel, move, raise from contact:

2024_02_25_linux_cn_hover_button_click_touch_release_wait_hover

The only real distinction is the alternating signal, which is on like 0x0a 2&3 e&f (both first and second window). We could detect that for this pen and 'solve it'.

The problem is that the SP pen on linux creates a binary code (probably a pen identifier, just like the M2 on Windows, see this section and we don't want that binary code to trigger the eraser detection. We could do something ugly where we check if 6/6 frames were perfectly alternating or something, but I'm really worried that this binary code is like the 'marker color', a unique id for each pen to allow the screen to distinguish two of them and some pens may just have an id that creates a binary sequence that perfectly alternates. In short; I don't yet know how to solve this, only thing I can think of is making the eraser signal trump the button action... We could explore that, I don't think I've ever had an accidental eraser action, but the whole intent of this PR is to use the strong binary indicators available in v2+.

I had hoped the whole design in the CN pen with the whole eraser & button having almost the same signal because the emitter end is actually facing the screen was just a cost cutting measure, but according to this document the eraser in a side button is an approved design.

Funny thing is that this section then describes that the protocol can't actually handle that well:

Unlike tail-end eraser implementations, button-based implementations can physically allow the user to activate/deactivate the erase affordance without the pen transitioning through the "out of range" state. However, this is not supported by the underlying protocol.

Instead, rely on magnitude for determining if the v2 signal is present.
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
src/core/generic/dft.hpp Outdated Show resolved Hide resolved
@iwanders
Copy link
Contributor Author

A few quick notes before work from my testing yesterday evening in comments.

Another limitation that currently affects mpp v2 pens is that the tilt is currently taken from Position, row 1, but that signal is only present when the stylus is touching the screen. I wonder if we should process Position2 if present instead. That has a solid signal on the tilt. Or we can - when we process Position2 for the contact detection store the dft values of the tilt row and consume that in the normal Position processing.

@StollD
Copy link
Member

StollD commented Feb 27, 2024

Another limitation that currently affects mpp v2 pens is that the tilt is currently taken from Position, row 1, but that signal is only present when the stylus is touching the screen.

Do you know how this is handled on Windows? On the "old" devices without a DFT stylus, the tilt is 0 when the pen tip is not touching the display.

src/core/linux/errors.hpp Outdated Show resolved Hide resolved
@iwanders
Copy link
Contributor Author

Another limitation that currently affects mpp v2 pens is that the tilt is currently taken from Position, row 1, but that signal is only present when the stylus is touching the screen.

Do you know how this is handled on Windows? On the "old" devices without a DFT stylus, the tilt is 0 when the pen tip is not touching the display.

I checked, I'm really surprised, but it does indeed become zero if the stylus isn't touching the screen. Even Microsoft's DigiInfo reports zero tiltx and tilty if the stylus isn't in contact. I installed Krita on windows, with the input API Windows 8+ Pointer Input (Windows Ink) this is also the behaviour. Same with the pointer events webapp.

There's no technical reason though... Like we do have the data in Position2 row 1 when the pen is raised, did a quick hack with just doing:

		case ipts::protocol::dft::Type::PositionMPP_2:
			this->handle_position_mpp_2(dft);
			this->handle_position(dft);
			break;

And in that case I do end up with a tilt that updates while the pen is raised, both in Krita as in the pointer events webapp. I'm astonished Windows doesn't do this.

Also, speaking about dft updates... we currently call on_stylus every dft window:

this->process_stylus(m_dft.get_stylus());

Should we think about a way where we can ensure that we only send that once for each group? Such that we don't have to worry about keeping the stylus data in a consistent state as we process the various dft windows in one group? Like, currently we consume the previous' cycle 0x0a DFT window on the next group's Button window. It's not a huge concern imo, but something to think about.

@StollD
Copy link
Member

StollD commented Feb 27, 2024

Tilt is calculated from two position signals, right? How does this work when one of the transmitters is out of range? Maybe thats why they only use the data when it is touching the display: To make sure both transmitters are in proximity.

Or this is just some weirdness that they inherited from N-Trig and never bothered to change.

I think by default iptsd should follow the Windows behaviour, but this sounds like a useful thing to put behind a config option. Although maybe that should be a seperate PR?

Should we think about a way where we can ensure that we only send that once for each group? Such that we don't have to worry about keeping the stylus data in a consistent state as we process the various dft windows in one group? Like, currently we consume the previous' cycle 0x0a DFT window on the next group's Button window. It's not a huge concern imo, but something to think about.

Another thing I noticed on my SB2 is that it only updates the tilt if I move the stylus. It sounds stupid with how precise these coordinates are supposed to be (1/9600 of the screen), but I can carefully tilt the stylus without the value changing.

Maybe that is something that the DFT parser could emulate.

@iwanders
Copy link
Contributor Author

Tilt is calculated from two position signals, right? How does this work when one of the transmitters is out of range? Maybe thats why they only use the data when it is touching the display: To make sure both transmitters are in proximity.

If either signal strength goes below a threshold we just lose the pointer position and/or tilt signal. Thing is, in Krita you can see the outline of the cursor update to show the tilt and size of the brush before you start drawing, so there's a lot of value in updating the tilt before you touch the screen imo.

I think by default iptsd should follow the Windows behaviour, but this sounds like a useful thing to put behind a config option. Although maybe that should be a seperate PR?

Yeah, we can do that, only real outstanding items with this piece of work:

  • a bit more testing, ideally with some others with Slim Pen2 & SP7+, but it may be easier to consume for others after it is merged.
  • The lack of eraser functionality with my Chinese pen, but the only solution I see there is being able to let the eraser disable the button state. Thoughts on this?

It sounds stupid with how precise these coordinates are supposed to be (1/9600 of the screen)

I expect that's part of the filtering done by the driver / hardware... I wish the hardware just did the whole position & tilt estimation, it would make things much easier. On my SP9 the current position estimate algorithm is not ideal, at times it seems to jump towards the edge of an antenna, particularly at some specific tilt angles. Seems less of a problem with other pens though, the Slim Pen 2 seems particularly susceptible to this problem.

@StollD
Copy link
Member

StollD commented Feb 28, 2024

  • The lack of eraser functionality with my Chinese pen, but the only solution I see there is being able to let the eraser disable the button state. Thoughts on this?

This is more like a hot thought and not really thought through, but can you let one method guide the other one? For example, if the new method detects a button press, you ignore the button magnitude setting but do the same phase calculations to determine button / rubber?

I don't know how reliable this would be though (because I don't really know the issue with the original method in the first place).

EDIT: Wait, or do you mean that you can detect the eraser already, but you want to press eraser and button at the same time? I am not actually sure what my SB2 would do in such case. But if necessary I would be fine with the eraser disabling the button.

@StollD
Copy link
Member

StollD commented Feb 28, 2024

From the SB2:

  • With a rear mounted eraser, the button gets disabled in eraser mode.
  • With an eraser button on the side, pressing the button disables the eraser.

The mppv2 button detection can also happen when the eraser is pressed through
a thumb button. So instead of assuming it is a barrel button, use the phase to
decide whether it is the eraser or barrel button.
src/core/generic/config.hpp Outdated Show resolved Hide resolved
@iwanders
Copy link
Contributor Author

I don't know how reliable this would be though (because I don't really know the issue with the original method in the first place).

Oh, mea culpa! I should've totally explained that in the description. So basically, the signal to noise ratio in the DftButton row 0 is kinda poor, for example in this test, the button isn't actually pressed, but the 0th row of the DftButton window clearly lights up. This is what causes the ghost button presses that slim pen 2 users have been reporting, I tried cranking the threshold up, as was suggested in one of the threads, while that resolved the ghost button presses it made the button detection only work when the tip was practically touching the screen for me. The 0th row is even more noisy at roughly 20 degrees towards the screen, and it is worse when the barrel button side is pointing towards the screen. I think it's still strong enough for the phase detection, I would've preferred a nice 'binary' signal, but maybe this is the best we have for now.

This is more like a hot thought and not really thought through, but can you let one method guide the other one? For example, if the new method detects a button press, you ignore the button magnitude setting but do the same phase calculations to determine button / rubber?

Yep! I truly appreciate your suggestion, I think this works really well. I just pushed this in 2d49ff7, I changed m_mppv2_button into m_mppv2_button_or_eraser, then we can just use the phase check to discern between barrel button and eraser button. I got too stuck on the 'button detection' and didn't think of that we can also just use it as a 'button or eraser' detection.

This seems to work for all 4 pens I have here, all of them allow eraser detection as well as button detection without any changes to settings.

but you want to press eraser and button at the same time?

Nope, you understood it right. I don't see much use in supporting both the button and the eraser.

Current solution is nice; the new changes are all additive. If they cause problems they can also all be disabled by increasing the new magnitudes in the configuration. I don't expect problems of course, but it's nice to know that we have the knobs to tune if it does. I made one more comment on a variable name, I think that should be prefixed. I'll try to do some more testing this evening and over the next few days. Aiming to move this out of draft at the beginning of the weekend if I don't discover any issues during my testing.

We should also really consider making an application that just prints the relevant dft magnitudes, doesn't need to be pretty, but currently it is impossible to say - print the button magnitudes - without doing any code changes. Happy to file a PR for this if you're ok with something simple in this direction.

@StollD
Copy link
Member

StollD commented Feb 29, 2024

I tried cranking the threshold up, as was suggested in one of the threads, while that resolved the ghost button presses it made the button detection only work when the tip was practically touching the screen for me.

Thanks for the explanation! I figured that it must have had something to do with signal strength, but I didn't consider the distance from the screen.

This seems to work for all 4 pens I have here, all of them allow eraser detection as well as button detection without any changes to settings.

Nice!

We should also really consider making an application that just prints the relevant dft magnitudes, doesn't need to be pretty, but currently it is impossible to say - print the button magnitudes - without doing any code changes. Happy to file a PR for this if you're ok with something simple in this direction.

I have been thinking about how I would do a DFT calibration tool for a while, but I never got around to working on it because of other stuff. A simple debugging app that just prints the magnitudes would be a great start in that regard.

@iwanders iwanders marked this pull request as ready for review March 2, 2024 12:34
@iwanders
Copy link
Contributor Author

iwanders commented Mar 2, 2024

Marked this as ready for review, it's good to go in imo: Did some more testing yesterday evening and Wednesday evening. The code we arrived at seems to work well for all 4 pens.

@StollD StollD merged commit a34eaaa into linux-surface:master Mar 3, 2024
@quo
Copy link
Contributor

quo commented Mar 4, 2024

Amazing work! I never even considered that the type 0xa windows would contain a button signal, I assumed it was a pen ID or something, since it just seemed to cycle through the same three six values (and I don't have a MPP2 pen myself).

Code looks good to me. My only concern is that if the first type 0xa DFT window is missing for some reason, you will incorrectly read the button signal from the second window. So it would be better to just store the first 0xa DFT window, and delay the processing until you see the second one, so you know you're using the right window.

BTW, any chance you could share your raw data captures? When I have some more time I may try to figure out what the other 0xa bits mean.
(Ideally separate captures of hovering, contact, eraser hover, eraser contact, and button press for each pen.)

@iwanders
Copy link
Contributor Author

iwanders commented Mar 5, 2024

BTW, any chance you could share your raw data captures? When I have some more time I may try to figure out what the other 0xa bits mean.

Certainly @quo , I've added a lot of it just now in a release associated to my analysis repository: https://github.com/iwanders/analysis-ithc-iptsd/releases/tag/v0.0.1

I hope you'll find that sufficient, it's the raw IRPMon recordings from Windows, my repo contains scripts to convert those to the iptsd binary files.

I think the 0xa bits identify the pens with some unique id. I'm not sure how relevant that is. What currently still feels a bit wonky for the SP2 is the actual position estimates, in particular with the pen at an angle.

Another (probably more fun) thing to look at would be the way the pressure is encoded digitally, I looked at it briefly here, it does seem to contain a delta in each frame. I didn't figure it out though.

Lots of interesting things in this data, but ultimately I had to prioritize fixing the button glitching over exploring all the other information as time as limited. I'll have to step away from the Surface stuff to do some other things in my spare time, but if you do make some discoveries do ping me!

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

Successfully merging this pull request may close these issues.

3 participants