SuperFlyTV / xkeys

A Node.js module to interact with the X-keys panels.
MIT License
33 stars 12 forks source link

Add support for the VEC Footpedal #89

Closed peternewman closed 1 year ago

peternewman commented 1 year ago

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

nytamin commented 1 year ago

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?

nytamin commented 1 year ago

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.

peternewman commented 1 year ago

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: https://github.com/SuperFlyTV/xkeys/blob/bcfb831bc9fb76ab1e384b2e4394816e7648fb2b/packages/core/src/xkeys.ts#L74

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 commented 1 year ago

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 commented 1 year ago

Probably best as P.I. Engineering designs many USB products that are not X-keys, the VID is not sufficient to define the X-keys product line.


From: Johan Nyman @.> Sent: Wednesday, November 29, 2023 2:47 AM To: SuperFlyTV/xkeys @.> Cc: Michael Hetherington @.>; Mention @.> Subject: Re: [SuperFlyTV/xkeys] Add support for the VEC Footpedal (PR #89)

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.

— Reply to this email directly, view it on GitHubhttps://github.com/SuperFlyTV/xkeys/pull/89#issuecomment-1831378113, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AS6L3QSTCAA742ZC5FGJAMLYG3SBXAVCNFSM6AAAAAA3KD7L5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZRGM3TQMJRGM. You are receiving this because you were mentioned.Message ID: @.***>

nytamin commented 12 months ago

@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.