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.

Sign up for GitHub

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

Jump to bottom

Cleanup to C Plugins #66

Merged
merged 6 commits into from
Oct 25, 2024
Merged

Cleanup to C Plugins #66

merged 6 commits into from
Oct 25, 2024

Conversation

simon-wh
Copy link
Member

@simon-wh simon-wh commented Nov 10, 2023 •
edited
Loading

  • Remove need to allocate DeviceInfo in rust from a c plugin. We just let them use the FFI type and we copy the data over
  • Eliminate _prefix on some funcs
  • Remove dangling pointer to callback_data on cplugin unload
  • Add some const qualifiers to things that should never be mutated
  • Cleanup some warnings from instances of Box::from_raw that were intended to drop the object, but weren't calling drop on them

TODO:

@Sainan
Copy link
Contributor

Sainan commented Nov 10, 2023 •
edited
Loading

Updating the "wooting-analog-plugin-examples" repo should be as easy as removing the _ from the function names, since the ANALOG_SDK_PLUGIN_ABI_VERSION is defined by the plugin.h.

Also, the WootingAnalog_DeviceInfo type may have to be replaced with the WootingAnalog_DeviceInfo_FFI type.

simon-wh and others added 3 commits October 21, 2024 18:00
@simon-wh
Cleanup cplugin interface to fully use DeviceInfo_FFI and eliminate _…
7c95429 
… prefix on some funcs

- Remove dangling pointer to callback on cplugin unload
- Remove need to allocate DeviceInfo in rust from a c plugin. We just let them use the FFI type and we copy the data over
- Add some const qualifiers to things that should never be mutated
@Sainan @simon-wh
Further C plugin cleanup (#67)
304f92f 
* Simplify CMakeLists for test_c_plugin
It no longer needs linking against Rust code

* Remove new_device_info & drop_device_info

* Bump CPLUGIN_ABI_VERSION since everything's definitely incompatible now
@Sainan @simon-wh
Fix test (hopefully) (#68)
2d77f3d 
* Fix "bindings changed"

* Also run workflow on PR
@simon-wh
Copy link
Member Author

@Sainan Do you see any issues with the state of this PR itself? Would be good to get this merged now

@Sainan
Copy link
Contributor

Sainan commented Oct 24, 2024

Seems fine, and double-checked it works with my plugin.

@Sainan
Copy link
Contributor

Sainan commented Oct 24, 2024 •
edited
Loading

I'd also be ready and able to update the example C plugin. Afaict, it's already broken because it doesn't seem to export the ABI version constant, at least on Windows. But would be a bit pointless for me to submit ABI v0 patches now.

Although you could remove the #pragma comment(lib, ...) from the plugin.h now.

@simon-wh simon-wh merged commit afbf6e1 into develop Oct 25, 2024
2 checks passed
@simon-wh simon-wh deleted the feature/c-plugin-cleanup branch October 25, 2024 09:16
@simon-wh
Copy link
Member Author

I'd also be ready and able to update the example C plugin. Afaict, it's already broken because it doesn't seem to export the ABI version constant, at least on Windows. But would be a bit pointless for me to submit ABI v0 patches now.

Although you could remove the #pragma comment(lib, ...) from the plugin.h now.

If you're able to create PRs for updating those bits, that would be a massive help 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simon-wh @Sainan