Move hid-rp code dependency out of this project

[benedekkupper]

Currently the hid-rp external dependency is added as a git submodule. The problems I see with this:

1. wrong license information is provided in idf_component.yml
2. updates need to be pulled in manually
3. generated headers are committed

I propose that we add the idf_component.yml in its home repository, which can be depended on from here.

[finger563]

Hey @benedekkupper thanks for the comment!

1. Ah good point - I'll update the license information in the espp/hid-rp component manifest
2. That's actually one of the reasons I like using submodules because it provides a direct mechanism to ensure breaking changes aren't accidentally introduced without additional testing of downstream dependencies
3. Yes - right now that's to simplify the use of the library.

I do agree it would benefit the community for you to add a component manifest file to the source hid-rp repository that you maintain, though the upstream has introduced a breaking change w.r.t. to the version that is pointed to by this submodule, so for now (until a move to espp v2.x) I'll keep this submodule pinned to the version it's on.

[finger563]

Also @benedekkupper - based on my understanding, MPL-2.0 is compatible with secondary licenses such as MIT (which I use for this code). Also, the original MPL-2.0 license is provided (via the submodule with its originating code, unmodified), so as far as I understand that's permissible right? If that's the case, then I'll probably leave the MIT license on the component (which adds various gamepad related wrappers) since the MPL-2.0 license / code is properly accessible and distributed. Does that make sense?

[benedekkupper]

I do agree it would benefit the community for you to add a component manifest file to the source hid-rp repository that you maintain, though the upstream has introduced a breaking change w.r.t. to the version that is pointed to by this submodule, so for now (until a move to espp v2.x) I'll keep this submodule pinned to the version it's on.

I can branch off from that revision and add a component file there - not optimal, but a workable solution. I can also start using proper API versioning, so you can always anchor to versions that you depend on.

MPL-2.0 is compatible with secondary licenses such as MIT (which I use for this code). Also, the original MPL-2.0 license is provided (via the submodule with its originating code, unmodified), so as far as I understand that's permissible right?

Yes, I'm not claiming any breach of copyright, I only mean that for consumers of the component it might not be clear that the component source code they pull in, is not pure MIT licensed, but a mix of two licenses with different characteristics.

[benedekkupper]

And an unrelated note, but worthy of mention: throughout the examples and documentation, you create the raw_descriptor (as a non-const local variable), then you copy the contents to a std::vector<uint8_t>, what's the rationale behind that? raw_descriptor is a std::array, so you can use .data(), .size(), .begin() and .end(). Moreover, if you mark it as static constexpr, then you effectively reduce your RAM use to 0, and also make sure that the descriptor's lifetime extends beyond local scope.

[finger563]

@benedekkupper

I do agree it would benefit the community for you to add a component manifest file to the source hid-rp repository that you maintain, though the upstream has introduced a breaking change w.r.t. to the version that is pointed to by this submodule, so for now (until a move to espp v2.x) I'll keep this submodule pinned to the version it's on.

I can branch off from that revision and add a component file there - not optimal, but a workable solution. I can also start using proper API versioning, so you can always anchor to versions that you depend on.

If you do that and publish that version to the registry with a semver code, then I can try to get my component to simply depend on that one and we can make sure that works, then in the future (as you make new releases, I'll test updates to them to ensure that they're compatible and it should be fine :)

If we go that route, I think for this version espp v1.x I still need to keep the generated headers in my include/hid/pages directory to maintain compatibility, unless you have other proposals?

Then in the next versions, we can go to autogenerating them as part of the build system (though I'm always a little cautious around that, since much of the main build happens in CI, with little opportunity for inspection and control).

MPL-2.0 is compatible with secondary licenses such as MIT (which I use for this code). Also, the original MPL-2.0 license is provided (via the submodule with its originating code, unmodified), so as far as I understand that's permissible right?

Yes, I'm not claiming any breach of copyright, I only mean that for consumers of the component it might not be clear that the component source code they pull in, is not pure MIT licensed, but a mix of two licenses with different characteristics.

Ah I see - thanks! I'll make a PR which indicates MIT + MPL-2.0 as the license, do you think that would be good?

[finger563]

And an unrelated note, but worthy of mention: throughout the examples and documentation, you create the raw_descriptor (as a non-const local variable), then you copy the contents to a std::vector<uint8_t>, what's the rationale behind that? raw_descriptor is a std::array, so you can use .data(), .size(), .begin() and .end(). Moreover, if you mark it as static constexpr, then you effectively reduce your RAM use to 0, and also make sure that the descriptor's lifetime extends beyond local scope.

Ah that's likely just me being lazy / not having great habits 😅 There are definitely some cases that I end up having to do that anyway since we need to dynamically switch between report descriptors among subclasses, so it can no longer be static, but def. a good callout to update the examples / doc for better use and highlight the tradeoffs.

I'd def. be open to a PR to help in that respect if you're willing 🙏 :)

[benedekkupper]

If we go that route, I think for this version espp v1.x I still need to keep the generated headers in my include/hid/pages directory to maintain compatibility, unless you have other proposals?

Then in the next versions, we can go to autogenerating them as part of the build system (though I'm always a little cautious around that, since much of the main build happens in CI, with little opportunity for inspection and control).

I never understood the motivation behind your choice to commit the generated files, as the code generator only needs cmake and python itself to work, which are present on any (usable) CI. If you take a look at hid-rp's own CI, there is no extra image used or tool installed. ESP-IDF itself also requires these tools to be available, so it's highly unlikely that someone cannot use the library due to the code generator not working. I think the generated code can be removed without any worries.

Ah I see - thanks! I'll make a PR which indicates MIT + MPL-2.0 as the license, do you think that would be good?

If we manage to split the library and examples to separate components, that won't be necessary.

[finger563]

I think originally I was under the impression that your code was fetching the latest json files, parsing them, then generating the c++ files - but looking over it again I think it's actually just parsing versioned files, which is much easier to control / monitor. Of course having the files locally helps with language servers and such, but it's not a huge issue.

Sounds good - for now I'll make the change anyway, so that current users and people who use this version will have the correct info, and we can easily update that once we make the other changes.