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.

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

Add Larod #120

Draft
wants to merge 53 commits into
base: main
Choose a base branch
from
Draft

Add Larod #120

wants to merge 53 commits into from

Conversation

JsGjKJzi
Copy link
Contributor

The acap-rs workspace doesn't currently provide wrappers for the larod library. When it is ready, I hope this merge request will provide that.

Immediate goals:

  • Provide safe Rust wrappers around the Larod functions, and abstract into rust structs and impls where appropriate.
  • Provide tests to verify functionality.

Since larod uses peripheral hardware, I plan to build the tests and execute them on an Axis P1468-LE.

Checklist before requesting a review

  • I have performed a self-review of my own code
  • I have verified that the code builds perfectly fine on my local system
  • I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have verified that my code follows the style already available in the repository
  • I have made corresponding changes to the documentation

@JsGjKJzi
Copy link
Contributor Author

Just adding a comment that I am still relatively new to Rust, so suggestions to improve my code is very welcome. I will also be learning about functionality of the larod library during the process of creating this wrapper. So, guidance on interfacing with larod is also very welcome.

@JsGjKJzi
Copy link
Contributor Author

Looking at the larodError documentation a bit more closely, I realize what I have so far won't work, and maybe larod cannot be wrapped?

https://axiscommunications.github.io/acap-documentation/docs/api/src/api/larod/html/structlarodError.html

Notably larodError, which is passed to nearly every function, must be uninitialized. It might be impossible for rust to produce such a pointer https://doc.rust-lang.org/std/mem/union.MaybeUninit.html.

@apljungquist
Copy link
Contributor

Since you say the code is not ready I will focus on answering your questions and assist you when you ask for it for now. Consequently I haven't looked much at the code but I address your comments below.


Just adding a comment that I am still relatively new to Rust

Diving in at the deep end with unsafe. A word of warning; In my opinion unsafe combines the worst of C(++) and Rust, and writing safe wrappers is especially frustrating since many of the safety aspects that we need to guarantee are not documented - sometimes not even considered - by the library authors. To make matters worse, we often cannot even test empirically if our code is correct because UB does not mean that it will segfault or do anything weird right now, it means it could do it now or at any point in the future. So, know know what you are getting yourself into, that it is ok to quit and don't let this experience discourage you from using Rust because there's another side to the language that is carefree and fun 🌞 🌈

What is your programming background? I will try to adjust my communication accordingly.

I will also be learning about functionality of the larod library during the process of creating this wrapper.

That makes two of us.

Notably larodError, which is passed to nearly every function, must be uninitialized.

I have found that sometimes the documentation should be taken with a grain of salt (I have found errors) and other times it is important to read it carefully. In any case the examples can be a good supporting resource. in vdo_larod.c 1 we find this:

larodError* error = NULL;
// ...
if (!larodConnect(&conn, &error)) {

We clearly don't pass an uninitialized variable but a pointer to null. I think this is what the documentation means when it says "An uninitialized handle to an error".

Compare this to send_event.c 2 which is makes heavy use of glib and, presumably, follows glib error conventions:

GError* error = NULL;
// ...
if (!ax_event_handler_declare(event_handler,
                                  // ...
                                  &error)) {

Footnotes

  1. https://github.com/AxisCommunications/acap-native-sdk-examples/blob/9b00b2fdf23672f8910421653706572201c2ed8b/vdo-larod/app/vdo_larod.c#L85

  2. https://github.com/AxisCommunications/acap-native-sdk-examples/blob/9b00b2fdf23672f8910421653706572201c2ed8b/axevent/send_event/app/send_event.c#L170

@JsGjKJzi
Copy link
Contributor Author

Thanks for the feedback. My background is varied engineering (R&D, so jack of all trades master of none sort of thing). I've written production C++ for embedded microcontrollers, C++ for AI using OpenCV and libtorch, lots of Python, and a few web backend type services using Rust. Again, I'm no expert, but I've done a lot of dabbling in various things.

I'm ok diving into the deep end. Just more to learn hopefully :).

One piece from the larod docs that I worry a bit about is this quote, "error can also be NULL if one does not want any error information." I worry that if I initialize the pointer to Null, we'll just never get any error information back.

@apljungquist
Copy link
Contributor

I'm comparing to axevent again because I recently worked on those bindings.

I think this is what passing a null pointer would look like: https://github.com/AxisCommunications/acap-native-sdk-examples/blob/9b00b2fdf23672f8910421653706572201c2ed8b/axevent/send_event/app/send_event.c#L167

Compare that to passing a (not-null) pointer to a null pointer: https://github.com/AxisCommunications/acap-native-sdk-examples/blob/9b00b2fdf23672f8910421653706572201c2ed8b/axevent/send_event/app/send_event.c#L176

… the pointer to it is non-NULL.

- Added some documentation.
- Mimicked the try_from macro from the axevent crate to DRY the code.
- Stubbed out functions for Session (larodConnection)
- Start implementing a few functions to list hand handle larodDevices
- Add testing documentation to README.md
- Add runner specification to .cargo/config.toml (new)
- Set `aarch64-unknown-linux-gnu` to default build target.
- Add remote-test.sh script to run tests on remote target.
@JsGjKJzi
Copy link
Contributor Author

JsGjKJzi commented Nov 2, 2024

@apljungquist I just set up the P1468-LE camera and set up a remote testing workflow. I've added some documentation and configuration files in this commit.

It might be useful for you in your endeavors. I have confirmed it works for the larod library so far on the P1468-LE running OS 11.11.73. It might be worth considering as a cherry pick, or I could open a separate merge request for it.

Finishedtest` profile [unoptimized + debuginfo] target(s) in 1.15s
Running unittests src/lib.rs (/workspaces/acap-rs/target/aarch64-unknown-linux-gnu/debug/deps/larod-d753965a4827e400)
larod-d753965a4827e400 100% 6363KB 9.9MB/s 00:00

running 10 tests
test tests::it_creates_larod_map ... ok
test tests::larod_map_can_get_2_tuple ... ok
test tests::it_drops_map ... ok
test tests::larod_map_can_get_4_tuple ... ok
test tests::larod_map_can_get_str ... ok
test tests::larod_map_can_get_int ... ok
test tests::larod_map_can_set_int ... ok
test tests::larod_map_can_set_4_tuple ... ok
test tests::larod_map_can_set_2_tuple ... ok
test tests::larod_map_can_set_str ... ok

test result: ok. 10 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
`

@apljungquist
Copy link
Contributor

apljungquist commented Nov 2, 2024

Neat. I didn’t know about runner so I implemented something similar in cargo-asap-sdk. I’m traveling over the weekend but when I’m back I’ll try out your solution and think about how we could leverage it.

I currently think of tests in two categories:

  • Library test. This is what you have written and implemented support. We typically upload them to /tmp/ and run them as root. Starting with AXIS OS 12.0 this will be difficult or impossible to do
  • Application tests. These can be packaged as an EAP and installed on the device as any app. They run as the same user as the app would, which is required for APIs like message broker. This likely will continue to work or, in the worst case, the test output would have to be retrieved from the system logs.

Were you aware of cargo-acap-sdk test? Asking because if not then I have a marketing problem.

Copy link
Contributor

@apljungquist apljungquist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the simplicity of this script but before I make any suggestions about how to upstream it I have a couple of questions to make sure I understand it.

remote-test.sh Outdated Show resolved Hide resolved
remote-test.sh Outdated Show resolved Hide resolved
- Change error handling to better check for a non-NULL larodError pointer.
- Add some documentation for LarodMap functions.
- Set LarodMap functions to public.
- Add some additional tests.
- Hide on-device tests behind a device-tests feature.
- Add a basic example.
@JsGjKJzi
Copy link
Contributor Author

JsGjKJzi commented Nov 6, 2024

Were you aware of cargo-acap-sdk test? Asking because if not then I have a marketing problem.
I might have been, but have been caught up in thinking through the API I'd like to achieve that I may have forgot about it. I have definitely used cargo-acap-sdk build to build an eap.

Using that may ultimately be a better approach than using the runner as discussed above.

@apljungquist
Copy link
Contributor

Happy to see you are still working on this 🙂 What's are you aiming for (the whole API vs some subset, a minimal safe abstraction vs highly ergonomic abstraction, integration with VDO, etc) and how far do you think you have come?
Is there anything I can do to help?

I'm sorry I still haven't made time to study the APIs and your implementation.

@JsGjKJzi
Copy link
Contributor Author

JsGjKJzi commented Dec 4, 2024

Yep, I'm still working on it when I find time. I was initially planning on doing the whole API, but I'm starting to realize some elements of the larod API don't make a ton of sense. I'm also thinking covering the whole API is a bit more work than I initially realized. So, I'm leaning more towards a minimal but functional library for now that strikes a balance between being a relatively thin wrapper and providing an idiomatic Rust API.

I am also starting to work on the VDO library so I can get frames from the camera. That might be a bit more difficult looking at the imageprovider.c file in the vdo-larod example. Setting up background threads and interfacing the GLib will take a bit of thinking through. I'm aiming for an API that would work something like this:

fn main() -> Result<()> {
    let stream = Stream::builder()
        .channel()
        .format()
        .width()
        .height()
        .build();
    stream.start();
    while let Ok(frame) = stream.next_frame() {
        ...
    }
    Ok(())
}

Example liblarod Confusion

The larodCreateTensors function exists and simply takes a size_t number of tensors to create. I believe the library allocates some vector-like data structure on the heap and hands back a pointer to that data structure. The docs say to call larodDestroyTensors which takes a pointer to the larodConnection.

These two functions have me a bit confused. the larodCreateTensors does not seem to require a larodConnection, so it would seem the lifetime is like 'static. However, since the data structure is a C-allocated thing, we have to hand it back to C to deallocate and we don't seem to have a good function for doing that. Other functions, such as larodCreateModelInputs take a pointer to the larodModel, for which a larodConnection is needed in the first place. So the lifetime of these tensors is inherently tied to the lifetime of the larodConnection. Fine. Also, when larodDisconnect is called, any allocated tensors can be deallocated since they can easily be associated with that initial larodConnection.

I did a quick test (I believe, I can't find it now) where I used larodCreateTensors, larodConnect, and larodDestroyTensors(larodConnection* conn, ... ) and got no errors. That seems to invite the possibility for any client to deallocate any other client's tensors as long as they have a pointer to the container of tensors, which doesn't seem particularly safe. Maybe there is a use case for that, but I thought I would leave out a Rust version of larodCreateTensors for now.

Code Conventions

One thing I was struggling with was the combination of supported preprocessor configurations. We know a-priori what backends support what kind of operations. So, I would really like to create a Rust Preprocessor API which encodes these supported combinations in the type system. We could then lean on the Rust type system to make it mostly impossible to code an incompatible combination of parameters. I couldn't find a nice way of doing it with Enums. The typestate pattern might be able to do it but looked like it was going to quite a bit of library code to do.

For now, I'm following the C library pretty closely with a builder pattern and doing the appropriate checks in a .build() method at runtime and returning an Err if an invalid combination is requested.

Create Resolution struct to help maintain clarity.
- Set tensor buffers
- Get tensor name
- Get tensor pitches
- Get tensor layout
- Add memmap2 dependency
- Create and store memory map object when Tensor.set_buffer() is called.
- Add Tensor.copy_from_slice() to copy data to memory mapped resource.
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