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
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions etc/presets/surface-pro-9.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[Device]
Vendor = 0x045E
Product = 0x0C52

[Stylus]
MPPVersion = v2

[DFT]
ButtonMinMag = 50000
ContactMinMag = 50000
PositionMinMag = 500
10 changes: 10 additions & 0 deletions src/core/generic/config.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ namespace iptsd::core {

class Config {
public:
// Enum to denote which Pen Protocol is being used.
enum class MPPVersion {
// Used for Microsoft Pen Protocol v1 compliant pens.
V1,
// Used for Microsoft Pen Protocol v2 compliant pens.
V2,
};

// [Config]
bool invert_x = false;
bool invert_y = false;
Expand Down Expand Up @@ -49,6 +57,7 @@ class Config {
// [Stylus]
bool stylus_disable = false;
f64 stylus_tip_distance = 0;
MPPVersion stylus_mpp_version = MPPVersion::V1;

// [DFT]
usize dft_position_min_amp = 50;
Expand All @@ -57,6 +66,7 @@ class Config {
usize dft_button_min_mag = 1000;
usize dft_freq_min_mag = 10000;
usize dft_tilt_min_mag = 10000;
usize dft_contact_min_mag = 10000;
f64 dft_tilt_distance = 0.6;

public:
Expand Down
92 changes: 90 additions & 2 deletions src/core/generic/dft.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,17 @@ class DftStylus {
i32 m_imag = 0;
std::optional<u32> m_group = std::nullopt;

// This is a bit of a hack, for the MPP v2 button detection we only
// care about the first 0x0a dft window, but there's two in the frame,
// here we keep track of the group when 0x0a was encountered, this
// allows comparing against this group and only using the first 0x0a
// frame.
std::optional<u32> m_dft_0x0a_group = std::nullopt;

// Boolean to track whether the pen is in contact, used by the method
// that processes the pressure frame.
bool m_mppv2_in_contact {false};

public:
DftStylus(Config config, const std::optional<const ipts::Metadata> &metadata)
: m_config {std::move(config)},
Expand All @@ -50,6 +61,12 @@ class DftStylus {
case ipts::protocol::dft::Type::Pressure:
this->handle_pressure(dft);
break;
case ipts::protocol::dft::Type::Position2:
this->handle_position2(dft);
break;
case ipts::protocol::dft::Type::Dft0x0a:
this->handle_dft_0x0a(dft);
break;
default:
// Ignored
break;
Expand Down Expand Up @@ -185,7 +202,10 @@ class DftStylus {
rubber = val > 0;
}

m_stylus.button = button;
// Only set the button value if we're using a v1 pen.
if (m_config.stylus_mpp_version == Config::MPPVersion::V1) {
m_stylus.button = button;
}
m_stylus.rubber = rubber;
}

Expand All @@ -206,11 +226,79 @@ class DftStylus {
m_stylus.contact = true;
m_stylus.pressure = std::clamp(p, 0.0, 1.0);
} else {
m_stylus.contact = false;
m_stylus.contact = false || m_mppv2_in_contact; // NOLINT
m_stylus.pressure = 0;
}
}

/*!
* 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.
Comment on lines +237 to +239
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.

*/
void handle_dft_0x0a(const ipts::DftWindow &dft)
{
if (m_config.stylus_mpp_version != Config::MPPVersion::V2) {
return;
}

if (dft.x.size() <= 5) { // not sure if this can happen?
m_stylus.button = false;
return;
}

// Second time we see the 0x0a frame in this group, skip it.
if (!dft.group.has_value() || m_dft_0x0a_group == dft.group)
return;

m_dft_0x0a_group = dft.group;

// Now, we can process the frame to determine button state.
// First, collapse x and y, they convey the same information.
const auto mag_4 = dft.x[4].magnitude + dft.y[4].magnitude;
const auto mag_5 = dft.x[5].magnitude + dft.y[5].magnitude;
const auto threshold = 2 * m_config.dft_button_min_mag;

if (mag_4 < threshold && mag_5 < threshold) {
// Not enough signal, lets disable the button
m_stylus.button = false;
return;
}

// One of them is above the threshold, if 5 is higher than 4, button
// is held.
m_stylus.button = mag_4 < mag_5;
}

/*!
* Determines whether the pen is making contact with the screen, it can
* only be used for MPP v2 pens.
*/
void handle_position2(const ipts::DftWindow &dft)
{
if (m_config.stylus_mpp_version != Config::MPPVersion::V2) {
return;
}

// Set to false in case we return early.
m_mppv2_in_contact = false;

if (dft.x.size() <= 3) { // not sure if this can happen?
return;
}

const auto mag_2 = dft.x[2].magnitude + dft.y[2].magnitude;
const auto mag_3 = dft.x[3].magnitude + dft.y[3].magnitude;

const auto threshold = 2 * m_config.dft_contact_min_mag;
if (mag_2 < threshold && mag_3 < threshold) {
return;
}

// The pen switches the row from two to three when there's contact.
m_mppv2_in_contact = mag_2 < mag_3;
}

/*!
* Interpolates the current stylus position from a list of antenna measurements.
*
Expand Down
31 changes: 29 additions & 2 deletions src/core/linux/config-loader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ class ConfigLoader {
Config m_config {};
DeviceInfo m_info;

bool m_loaded_config {false};

public:
ConfigLoader(const DeviceInfo &info, const std::optional<const ipts::Metadata> &metadata)
: m_info {info}
Expand All @@ -48,14 +50,21 @@ class ConfigLoader {
* known working main system configuration.
*/
if (const char *config_file_path = std::getenv("IPTSD_CONFIG_FILE")) {
spdlog::info("Loading config {}, specified with IPTSD_CONFIG_FILE.",
config_file_path);
this->load_file(config_file_path);
return;
}

if (std::filesystem::exists(common::buildopts::ConfigFile))
if (std::filesystem::exists(common::buildopts::ConfigFile)) {
spdlog::info("Loading config {}.", common::buildopts::ConfigFile);
this->load_file(common::buildopts::ConfigFile);
}

this->load_dir(common::buildopts::ConfigDir, false);

if (!m_loaded_config)
spdlog::warn("No config file loaded at all, this is not good.");
}

/*!
Expand Down Expand Up @@ -95,6 +104,7 @@ class ConfigLoader {
continue;
}

spdlog::info("Loading config {}.", p.path().c_str());
this->load_file(p.path());
}
}
Expand Down Expand Up @@ -161,10 +171,12 @@ class ConfigLoader {

this->get(ini, "Stylus", "Disable", m_config.stylus_disable);
this->get(ini, "Stylus", "TipDistance", m_config.stylus_tip_distance);
this->get(ini, "Stylus", "MPPVersion", m_config.stylus_mpp_version);

this->get(ini, "DFT", "PositionMinAmp", m_config.dft_position_min_amp);
this->get(ini, "DFT", "PositionMinMag", m_config.dft_position_min_mag);
this->get(ini, "DFT", "PositionExp", m_config.dft_position_exp);
this->get(ini, "DFT", "ContactMinMag", m_config.dft_contact_min_mag);
this->get(ini, "DFT", "ButtonMinMag", m_config.dft_button_min_mag);
this->get(ini, "DFT", "FreqMinMag", m_config.dft_freq_min_mag);
this->get(ini, "DFT", "TiltMinMag", m_config.dft_tilt_min_mag);
Expand All @@ -175,6 +187,7 @@ class ConfigLoader {
this->get(ini, "Contacts", "SizeThreshold", m_config.contacts_size_thresh_max);

// clang-format on
m_loaded_config = true;
}

/*!
Expand All @@ -199,7 +212,21 @@ class ConfigLoader {
value = gsl::narrow_cast<T>(ini.GetReal(section, name, value));
else if constexpr (std::is_same_v<T, std::string>)
value = ini.GetString(section, name, value);
else
else if constexpr (std::is_same_v<T, Config::MPPVersion>) {
// Parse the pen protocol verison by first reading into a string.
const auto mpp_version = ini.GetString(section, name, "");
if (!mpp_version.empty()) {
if (mpp_version == "v1") {
value = Config::MPPVersion::V1;
} else if (mpp_version == "v2") {
value = Config::MPPVersion::V2;
} else {
throw common::Error<Error::ParsingInvalidValue> {
"Stylus mpp_version was not 'v1' or 'v2', got: " +
mpp_version};
}
}
} else
throw common::Error<Error::ParsingTypeNotImplemented> {typeid(T).name()};
}
};
Expand Down
3 changes: 3 additions & 0 deletions src/core/linux/errors.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace iptsd::core::linux {

enum class Error {
ParsingFailed,
ParsingInvalidValue,
ParsingTypeNotImplemented,
RunnerInitError,

Expand All @@ -25,6 +26,8 @@ inline std::string format_as(Error err)
switch (err) {
case Error::ParsingFailed:
return "core: linux: Failed to parse INI file {}!";
case Error::ParsingInvalidValue:
return "core: linux: Parsing encountered invalid value {}!";
case Error::ParsingTypeNotImplemented:
return "core: linux: Parsing not implemented for type {}!";
case Error::RunnerInitError:
Expand Down
1 change: 0 additions & 1 deletion src/ipts/parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ class Parser {
{
Reader reader(data);
reader.skip(header);

this->parse_hid_frame(reader);
}

Expand Down
2 changes: 2 additions & 0 deletions src/ipts/protocol/dft.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ constexpr u8 PRESSURE_ROWS = 6;

enum class Type : u8 {
Position = 6,
Position2 = 7,
Button = 9,
Pressure = 11,
Dft0x0a = 0x0a,
};

struct [[gnu::packed]] Metadata {
Expand Down