Add Larod #120

JsGjKJzi · 2024-10-29T15:58:47Z

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

… testing.

JsGjKJzi · 2024-10-29T16:01:31Z

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 · 2024-10-29T16:55:42Z

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 · 2024-10-29T18:28:15Z

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 ¹ 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 ² which is makes heavy use of glib and, presumably, follows glib error conventions:

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

JsGjKJzi · 2024-10-29T18:45:34Z

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 · 2024-10-29T19:41:56Z

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 · 2024-11-02T00:42:08Z

@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 · 2024-11-02T11:34:21Z

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.

apljungquist

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

- 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 · 2024-11-06T03:00:21Z

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.

…sion from which it was acquired.

apljungquist

Happy to see you are still making progress. I dropped a couple of comments on superficial stuff that came to mind while skimming. Do with them as you please.

apljungquist · 2025-02-12T20:13:29Z

crates/larod/src/lib.rs

@@ -589,7 +601,29 @@ impl<'a> Tensor<'a> {
            Err(maybe_error.unwrap_or(Error::MissingLarodError))
        }
    }
-    pub fn set_pitches() {}
+    pub fn set_pitches(&mut self, pitches: &[usize]) -> Result<()> {


Did you know you can make a function generic over N?

I haven't looked closely at what we are trying to do here, so I can't say if it would help or be wise, just some basic pattern matching going on in my brain.

https://doc.rust-lang.org/reference/items/generics.html#const-generics

apljungquist · 2025-02-12T20:17:16Z

crates/larod/src/lib.rs

+        P: AsRef<Path>,
+    {
+        let f = File::open(model_file).map_err(Error::IOError)?;
+        let Ok(device_name) = CString::new(<(M, H)>::as_str()) else {


Are you planning to use the result of as_str for anything other than creating a CString?
If not you may be interested to know that there are c-string literals these days.

JsGjKJzi · 2025-02-25T15:24:42Z

@apljungquist In general this crate is "done" in the sense that the vast majority of the functions and functionality exist. I have also documented most things. I didn't go into as much detail as the official documentation, but included a link to the Axis ACAP page. I guess I'll mark it as ready for review and I can address any shortcomings.

There are still a few things I'm not keen on, and a few that I'm not sure how best to implement.

Ergonomics

First, because the type implementing LarodModel owns its input and output tensors, and because we need to get a mutable reference to them to call copy_from_slice, we have to go through a dance to use them. It is particularly unsightly when running in a for loop, like looping over frames from VDO.

for buffer in running_stream.iter() {
    if let Some(ft) = preprocessor.input_tensors_mut().and_then(|it| it.first_mut()) {
        ft.copy_from_slice(frame_slice);
        job_request.run()?
    }
}

I was hopeful for something like this:

let preproc_tensor_iter = preprocessor.input_tensors_mut()?.iter_mut()?;
let preproc_input_tensor = preproc_tensor_iter.next()?
for buffer in running_stream.iter() {
  ...
}

But then, since preproc_input_tensor holds a mutable reference to the preprocessor, we can't easily access the output tensors.

Access Methods

Second, the larod library has a larodModel* larodGetModel( larodConnection *conn, const uint64_t modelId, larodError **error ) function that returns a model. I'm generally not sure how or why this function would be used. Especially since one would need to get the modelId via something like uint64_t larodGetModelId( const larodModel *model, larodError **error ) which requires that one have a pointer to the model in the first place.

Nevertheless, if one were to call larodGetModel, since both a Preprocessor and InferenceModel implement the LarodModel trait, it would have to be a function that returns some dynamic trait via something like

fn get_model(model_id: u64) -> Box<dyn LarodModel> {
    if preprocessor_model {
        Box::new(Preprocessor {})
    } else {
        Box::new(InferenceModel {})
    }
}

This would be a particularly involved function if we need to map the file descriptors and everything back into an InferenceModel for example. I'm not keen on that, but it could be done if you think it is necessary.

apljungquist · 2025-02-25T20:53:17Z

That's great news. I'll take a look at it this weekend.

It would be nice with an example app that demonstrates that it works and how to use it.

apljungquist

I think it's cool that we are getting close to having larod bindings and appreciate your effort. The PR is on the big side and it would be nice to split it up somehow to make it easier to reason about and keep moving forward. One thing that seems reasonable to chip off is the sys-crate, so I've focused on that this morning. I'm thinking that we can discuss it in this PR and once we are happy we isolate it and merge. What do you think?

On the topic of how to review, do you have any requests of suggestions for me?

apljungquist · 2025-03-01T11:31:56Z

crates/larod/examples/basic.rs

I don't have a camera at hand, but I guess these examples are meant to be executed using cargo run and relying on remote-test.sh for them to execute on the camera. Is that correct?
Would it be possible to package them as example apps?
Do you see any drawback to doing so?

Advantages in my opinion include:

It is consistent with the rest of the project where most programs that are intended to run on the camera are packaged as applications.

It communicates that the examples can run as normal applications with all the restrictions that entail, and shows how to set up a manifest that allows it.

apljungquist · 2025-03-01T11:33:46Z

crates/larod/examples/simple-file.rs

@@ -0,0 +1,141 @@
+//! # Simple example


Nice, detailed instructions. I look forward to trying them when I have a camera at hand.

apljungquist · 2025-03-01T11:46:22Z

crates/larod-sys/build.rs

+        .allowlist_recursively(false)
+        .allowlist_function("^(larod.*)$")
+        .allowlist_type("^(_?larod.*)$")
+        .default_enum_style(bindgen::EnumVariation::Rust {


I don't think we should use Rust style for enums that are returned from foreign functions because it would be undefined behavior if the foreign function adds a new variant. What do you think?

crates/larod-sys/Cargo.toml

apljungquist · 2025-03-01T11:54:13Z

crates/larod-sys/src/lib.rs

+#[allow(non_camel_case_types)]
+#[repr(u32)]
+#[derive(Debug)]
+pub enum FDAccessFlag {


Why are these types in the sys crate?

JsGjKJzi and others added 7 commits October 25, 2024 10:02

Add larod-sys crate

151cd8c

Add bindings.

8d3defa

Drop back to ACAP SDK 1.3. Get larod crate fleshed out a little to do…

709d58c

… testing.

Swap back to x86_64

e158693

Start implementing LarodMap

024e491

Implement get_string, get_int_arr2, get_int_arr4.

56d3720

Merge remote-tracking branch 'origin/main' into feature/larod

51e2518

JsGjKJzi added 5 commits October 29, 2024 23:44

- Improved error handling to always deallocate a larodError object if…

3d510b6

… the pointer to it is non-NULL. - Added some documentation. - Mimicked the try_from macro from the axevent crate to DRY the code.

Added builder pattern for LarodClient.

3e004a0

Add log.

021ea65

Start adding LarodDevice and Session

f9e0506

- Stubbed out functions for Session (larodConnection) - Start implementing a few functions to list hand handle larodDevices

Add remote testing

c05a0a1

- 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 added 2 commits November 1, 2024 22:54

Hide on-device tests behind a device-tests feature

e0fa140

Inverted remote-test.sh check for CARGO_TEST_CAMERA definition

147e438

apljungquist reviewed Nov 3, 2024

View reviewed changes

remote-test.sh Outdated Show resolved Hide resolved

remote-test.sh Outdated Show resolved Hide resolved

JsGjKJzi added 6 commits November 4, 2024 11:16

Update larod_sys/src/bindings.rs to match new enum style.

32db020

Change to EnumVariation::Rust as default bindgen enum style.

b3607bf

Fix test failure due to renamed method.

4309ed6

Remove note that is no longer needed.

de2367b

Run executable is no camera.

1dc6896

Explicitly tie the lifetime of LarodDevice to the lifetime of the Ses…

55a7158

…sion from which it was acquired.

apljungquist reviewed Feb 12, 2025

View reviewed changes

JsGjKJzi added 2 commits February 12, 2025 15:19

implement more Tensor methods

36eb55a

implement compile time definition of inference backend

96b2918

JsGjKJzi force-pushed the feature/larod branch from f422122 to 96b2918 Compare February 12, 2025 20:19

JsGjKJzi added 12 commits February 13, 2025 08:21

rewind Tensor file descriptors

98dbb90

merge main into feature/larod

0b43dd0

suppress name format lints

c62919c

add file access constants missed by bindgen

f58fd53

add fd_offset getter for Tensor

67f1602

add documentation

e19217a

add simple inference example

b80c40b

add more functions

e378419

fix a few missed things in last commit

56f42f6

incorporate clippy::pedantic lints

b196e7e

add remaining functions, add documentation, fix clippy lints

f3f7adb

adjust crate top level documentation

2a0642f

JsGjKJzi marked this pull request as ready for review February 25, 2025 15:24

JsGjKJzi requested a review from a team as a code owner February 25, 2025 15:24

JsGjKJzi added 2 commits February 25, 2025 16:34

re-export FDAccessFlag

01f4966

document session drop

43ed182

apljungquist requested changes Mar 1, 2025

View reviewed changes

JsGjKJzi added 3 commits March 3, 2025 11:55

add licenses

8453d8f

add newlines at end of files

96b0917

update crate versions to 0.0.0

3b28782

JsGjKJzi mentioned this pull request Apr 20, 2025

Add larod-sys #182

Merged

6 tasks

JsGjKJzi added 3 commits April 22, 2025 21:13

use NewType enums

79f6a7e

move custom code out of larod-sys

547bf88

alphabetize Cargo.toml

10ab960

Add Larod #120

Are you sure you want to change the base?

Add Larod #120

Uh oh!

Conversation

JsGjKJzi commented Oct 29, 2024

Immediate goals:

Checklist before requesting a review

Uh oh!

JsGjKJzi commented Oct 29, 2024

Uh oh!

JsGjKJzi commented Oct 29, 2024

Uh oh!

apljungquist commented Oct 29, 2024

Footnotes

Uh oh!

JsGjKJzi commented Oct 29, 2024

Uh oh!

apljungquist commented Oct 29, 2024

Uh oh!

JsGjKJzi commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apljungquist commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apljungquist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JsGjKJzi commented Nov 6, 2024

Uh oh!

apljungquist left a comment

Choose a reason for hiding this comment

Uh oh!

apljungquist Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

apljungquist Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

JsGjKJzi commented Feb 25, 2025

Ergonomics

Access Methods

Uh oh!

apljungquist commented Feb 25, 2025

Uh oh!

apljungquist left a comment

Choose a reason for hiding this comment

Uh oh!

apljungquist Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

apljungquist Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

apljungquist Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apljungquist Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JsGjKJzi commented Nov 2, 2024 •

edited

Loading

apljungquist commented Nov 2, 2024 •

edited

Loading