Skip to content

Support for C wrapper to enable other languages to call your library. - #11

Open
edancain wants to merge 53 commits into
lvauvillier:masterfrom
edancain:master
Open

Support for C wrapper to enable other languages to call your library. #11
edancain wants to merge 53 commits into
lvauvillier:masterfrom
edancain:master

Conversation

@edancain

Copy link
Copy Markdown

Hi Luc,
can't begin to thank you enough for this work, its impressive.

I am not a RUST developer, and have to learn quickly over the past couple of weeks. I have made editions to your code and changes where necessary to support implementation of the wrapper.

If you read the new readme.md ( /dji-log-parser/readme.md) there is an explanation as to what I have added to fork I made of your work. I have had to change a few use statements in files to support use serde::{Serialize, Deserialize};

Anyway, this now works for my purposes and I wanted to share back.

Thanks again,
Edan

edancain added 30 commits July 3, 2024 09:25
…de integration, calls to the static Rust parsing library. Woot woot
…ing it to disk, and io.Read read object in Go support.
…uired is simply to pass back the string pointer of the geojson created from the file parsing.

Renamed free_string for clarity and to avoid conflicts. Adjusted the get_last_error to work with the new structure. Ensured all public C API functions marked with #[no_mangle]

Moved teh C API specific functions into the c_api.rs file to keep it all clean.
…when building the lib, I have set it to build the .h file automatically and this breaks implementation in GO. Need to work this out. But for now, this works
Tests for go code
test data for go
testing with go the C wrapper
… the file is auto generated and exposes the correct functions from both c_api.rs and lib.rs
updates from main
merging from main
merging from main into master
merging from main into master
merging main into master
merging main into master
merging main into master
@lvauvillier

Copy link
Copy Markdown
Owner

Thank you very much for this PR and your kind words. I’m also learning Rust, and this is my first project :)

I quickly reviewed your changes and have a few initial comments / questions :

  • I think the right way is to create a new crate at the root of the project (e.g., dji-log-parser-c). You can check, I have added JS bindings on a wasm target in a separate crate a few days ago.
  • I see you added an option to the CLI to export keychains; this is definitely something I had in mind and planned to expose. I think it's better to place this code as a new Exporter. But this doesn't seem related to the C bindings, right?
  • Why did you need to add Deserialize?
  • Why did you need to change the AuxiliaryInfo struct?

@edancain

Copy link
Copy Markdown
Author

Hi Luc,
you are learning Rust? I would say you have mastered it. Me, this project of yours is the very first time I have ever used Rust, so my assumptions and code changes with regards to serde::{Serialize, Deserialize} and the AuxiliaryInfo struct were to fix build warnings really.

With Deserialize, I wanted to add symmetry to data handling and I wasn't sure at the start if I would need it to convert data back to into a Rust struct with a geojson file I had already created using the code. I just added it.

AuxiliaryInfo: I made changes to it due to the fact that version_data and signature_data were unused and probably giving me a warning. So I added the underscore to indicate to Rust that the field is intentionally there but unused, basically suppressing the compiler warnings. Wasn't sure if you were ever going to use them, but rather than removing them I just intentionally marked them as unused.

I am no expert with Rust, so many of my changes we the result of error messages when compiling it. My main additions to the code were simply to support the wrapper code as I learned what Rust needed. So, some of the changes are from initial attempts at it, and since it didn't break compilation they remained.

Greetings from New Zealand.

@akhenakh akhenakh mentioned this pull request Jul 19, 2024
@lvauvillier

Copy link
Copy Markdown
Owner

Ok, Thanks for your explanation. I agree there are unused variables, and we can address that by simply adding #[allow(unused)].

However, I don't think the PR is mergeable because you aim to export GeoJSON functionalities, whereas I want the bindings to mirror the original library's API—focusing only on parsing, keychain retrieval, and frame/record export.

The CLI crate serves primarily as an example of what can be achieved and how to work with the library.

Nevertheless, thank you for your efforts. This has provided a good starting point for me to understand C bindings, and I definitely plan to support this in the future.

Greetings from Paris.

@edancain

Copy link
Copy Markdown
Author

Hi Luc,
I agree with you that the PR is not mergeable because of what I want to add as functionality and you keeping it mirroring the original library API.
I couldn't get their API to build for me, so discovering your comments and your Rust work has been so helpful in my project and I can't thank you enough.
I'm very glad that what I have shown in the PR will help as a starting point in your understanding of C bindings.
Thank you so much!
Edan

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.

2 participants