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

Add support for the VEC Footpedal #89

Closed
wants to merge 5 commits into from

Conversation

peternewman
Copy link
Contributor

@peternewman peternewman commented Aug 9, 2023

I've only tested this as:
node packages/node/examples/basic-log-all-events.js

Copy link
Contributor Author

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

A few queries from me...

I don't know if @michaelhatPIengineering can assist?

packages/webhid/src/methods.ts Outdated Show resolved Hide resolved
Comment on lines +121 to +124
if (!this._firmwareVersionIsSet) {
rdData[1] = 214 // Fake initial message to set _firmwareVersion
} else if (!this._unitIdIsSet) {
rdData[1] = 3 // Fake initial message to set _unitId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first up and down events seem to get swallowed, can we not force extra messages here which I assume would solve this? Presumably this is also the same for the RailDriver?

Copy link
Member

Choose a reason for hiding this comment

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

Am I correct in assuming that this device doesn't really have the concept of a unitId? (That would make sense, since it won't accept the inititial write of _getVersion() and so won't return a unitId.)

If so, I suggest that we solve this by adding

if (this.deviceInfo.productId == 255) {
  this._unitId = 0 // VEC Footpanel doesn't support unitIds
  this._unitIdIsSet = true
}

into the init() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given it only ever returns the two bytes, and doesn't accept any writes, I don't think so! Although conversely I've never worked out what the second byte is for (some more generic underlying product/interface)

The device has no serial number via lsusb either (but maybe that's common to all xkeys?).

If so, I suggest that we solve this by adding

Do you think this would solve it missing the first keypress and release too when run as node packages/node/examples/basic-log-all-events.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If so, I suggest that we solve this by adding

if (this.deviceInfo.productId == 255) {
  this._unitId = 0 // VEC Footpanel doesn't support unitIds
  this._unitIdIsSet = true
}

into the init() method.

Adding that into the top of the init method (without removing my other changes) certainly doesn't break anything. Although the first press and release are still silently swallowed by the library currently.

[1, 2],
[1, 3],
],
disableButtons: [0],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is required or not, if I set it to 1 it does fire left pedal presses.

backLight2offset: 0,
//timestamp: none, VECFootpedal has no time stamp
btnLocation: [
[0, 0],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I leave this out, the right pedal causes exceptions.

rdData[1] = 0 // no pg switch, byte is always 0
}
rdData[2] = data.readUInt8(0) // remap button bits
rdData[3] = data.readUInt8(1) // remap button bits
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what, if anything, happens with this second byte?

@nytamin nytamin self-requested a review August 9, 2023 14:52
@nytamin
Copy link
Member

nytamin commented Aug 21, 2023

Thanks for this, @peternewman !

I reverted the changes to the XKeys.init() method, and simply accessed this.deviceInfo instead to avoid writing to the device in there.

Could you give this another spin to verify that it works well with your device before i merge it?

const pReceivedGenerateData = new Promise<void>((resolve) => {
this.receivedGenerateDataResolve = resolve
})
if (this.deviceInfo.productId !== 255) {
Copy link
Member

Choose a reason for hiding this comment

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

A question for you:
What happens if we try to write to the device? Does it crash? Or nothing?

I'm thinking that if it crashes we might want to guard against writing to it in some other methods as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I ever try and write to it, I get the following (so the device doesn't crash, but node does):

Error in XKeysWatcher TypeError: Cannot write to hid device
    at NodeHIDDevice.write (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/node/dist/node-hid-wrapper.js:19:21)
    at XKeys._write (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/core/dist/xkeys.js:652:25)
    at XKeys._getVersion (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/core/dist/xkeys.js:701:14)
    at XKeys.init (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/core/dist/xkeys.js:319:18)
    at setupXkeysPanel (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/node/dist/methods.js:77:17)
    at XKeysWatcher.handleNewDevice (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/node/dist/watcher.js:196:32)
    at XKeysWatcher.updateConnectedDevices (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/node/dist/watcher.js:181:22)
    at Timeout._onTimeout (/home/pi/companion-dev/xkeys-footpedals-fixed/packages/node/dist/watcher.js:136:22)
    at listOnTimeout (node:internal/timers:569:17)
    at process.processTimers (node:internal/timers:512:7)

So I did the protection in each place that calls write, as it often needed other steps to work around it, but maybe it should be done more centrally?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have been away, but earlier you asked if I could assist. Not sure if I can, the VEC foot pedal is a proprietary device made specifically for VEC and their software. It is not an X-keys so not sure if it belongs here. It is very limited in its capabilities, however, 3 bits is just 3 bits, so it does that. As you are finding there will be a lot of work arounds to make it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been away, but earlier you asked if I could assist. Not sure if I can, the VEC foot pedal is a proprietary device made specifically for VEC and their software.

Fair enough, I'd found lots of other hits on the Internet, and it's just integrated into Linux via the Event stuff, so I didn't realise it was originally designed just for VEC's software.

It is not an X-keys so not sure if it belongs here. It is very limited in its capabilities, however, 3 bits is just 3 bits, so it does that. As you are finding there will be a lot of work arounds to make it work.

That seems quite similar with the RailDriver ( https://xkeys.com/xkeys/rdproducts.html ) stuff too, looking at some of the workarounds for that in the code too.

I would say conversely, they're all P.I. Engineering devices, the VID confirms that, and the fact there's an official XKeys device which looks like later revisions of the VEC Infinity range it would seem a bit of a shame to me if the support wasn't added:
https://xkeys.com/xkeys/switches-pedals/footpedals.html
http://veccorp.com/foot-controls.html

@nytamin
Copy link
Member

nytamin commented Nov 2, 2023

Due to a combination of the added complexity needed (for preventing writes in many places) and that the VEC isn't an official xkeys product, I think that we won't merge this into the xkeys library, unfortunately.

I suggest that you instead clone the repo and make a (much smaller) library specifically to support the VEC pedal.

@nytamin nytamin closed this Nov 2, 2023
@peternewman
Copy link
Contributor Author

peternewman commented Nov 27, 2023

Due to a combination of the added complexity needed (for preventing writes in many places) and that the VEC isn't an official xkeys product, I think that we won't merge this into the xkeys library, unfortunately.

Obviously its your repo and you're welcome to do what you want, but I'd ask if you could please reconsider @nytamin as I think the following reasons are worth of further thought.

I think from the complexity perspective, there could potentially be just one block to the writes in writeData, and then maybe a little bit in init to fake the initialisation.

I suggest that you instead clone the repo and make a (much smaller) library specifically to support the VEC pedal.

Because it's got a PI Vendor ID, this is the obvious place to put it, and indeed was why I ended up here. I actually noticed the other day that Bitfocus Companion is already detecting the pedal and the library itself is throwing an error that it's not a known device and pointing me to here to report a bug, via this code:

`Unknown/Unsupported X-keys: "${deviceInfo.product}" (productId: "${deviceInfo.productId}", interface: "${deviceInfo.interface}").\nPlease report this as an issue on our github page!`

Also if I made a VEC specific library, then you'd have to have some pseudo code like:

if (PID == PI_Engineering_PID) {
  if (VID == VEC_Footpedal_VID) {
    do something with VEC specific library
  } else {
    do something with xkeys library
  }
}

And of course that change needs making to every single package or application that uses your library, which is at least all of these:
https://github.com/SuperFlyTV/xkeys/network/dependents
https://www.npmjs.com/package/xkeys?activeTab=dependents

Compared to adding a few more lines of special case which aren't dissimilar to the Rail Driver code to the library in one place and everyone benefits!

I'm happy to have another go at my changes to see if I can simplify them and put them just in writeData and a little bit in init if you'd then have another review of it and then maybe reconsider?

@nytamin
Copy link
Member

nytamin commented Nov 29, 2023

Ah, perhaps we should limit the devices that this library discovers to only the ones described in the products.ts file (since others won't work anyway). That way it shouldn't interfere with other libraries.

@michaelhatPIengineering
Copy link
Collaborator

@nytamin
Copy link
Member

nytamin commented Dec 6, 2023

@peternewman FYI, I have opened a PR that exposes a better way to filter for XKeys devices, please give it a read and a thumbs up if you think this solves the issue of using this library together with another library.

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