search

keyboardio / Kaleidoscope-Bundle-Keyboardio

A Kaleidoscope distribution for the Keyboardio Model 01 and other keyboards.
Other
17 stars 24 forks source link

Redundant HID implementation #20

Closed noseglasses closed 5 years ago

noseglasses commented 5 years ago

The files HID.h and HID.cpp are redundantly defined in library HID and KeyboardioHID. Diffing them shoes that both are almost identical. Is there a particular reason for this redundancy or could the HID library possibly be removed from the bundle?

obra commented 5 years ago

I recall something about this, but not much detail. Does git history tell you anything useful?

On Thu, Jun 13, 2019 at 5:46 AM noseglasses notifications@github.com wrote:

The files HID.h and HID.cpp are redundantly defined in library HID and KeyboardioHID. Diffing them shoes that both are almost identical. Is there a particular reason for this redundancy or could the HID library possibly be removed from the bundle?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/issues/20?email_source=notifications&email_token=AAALC2FU73SVA5QYEBNAUWTP2I6THA5CNFSM4HXZG4X2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GZJYCBQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2FUPEYTCNMPYQJLM2TP2I6THANCNFSM4HXZG4XQ .

algernon commented 5 years ago

I think the reason we have HID is that some parts of Arduino wants that, and were compiled before KeyboardioHID was pulled in. I may very well be wrong, though. I guess one way to find out would be to drop HID and see if anything breaks.

obra commented 5 years ago

Git history says:

commit 4df7064089175d5e97687e4f655a1e5a03ff477b Author: Gergely Nagy algernon@keyboard.io Date: Fri Apr 19 23:03:23 2019 +0200

Copy HID.h and HID.cpp over

We depend on our version of these, tightly. To make it easier to add new
features to the HID class itself, lets move it here. Arduino seems to

prefer the local copy over the built-in library, if both are present, so this should be safe.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

That suggests to me that we should be -removing- the separate HID library. Anyone feel like testing that? ᐧ

On Wed, Jun 19, 2019 at 8:20 AM Gergely Nagy notifications@github.com wrote:

I think the reason we have HID is that some parts of Arduino wants that, and were compiled before KeyboardioHID was pulled in. I may very well be wrong, though. I guess one way to find out would be to drop HID and see if anything breaks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/issues/20?email_source=notifications&email_token=AAALC2C6ZDUXIOJWLIXG6HDP3JFD7A5CNFSM4HXZG4X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYCG35I#issuecomment-503606773, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2CRXG6JVSRL5DROUJLP3JFD7ANCNFSM4HXZG4XQ .

noseglasses commented 5 years ago

I am going to give it a try.

noseglasses commented 5 years ago

The HID library is unused by the build. Two firmware binaries (default Model01 sketch) build with and without HID (after removing it) are identical. I submitted https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/21 that removes it.

algernon commented 5 years ago

Since #21 has been merged, this can be closed too.