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

The order of the tuples on a task getting inputs from other tasks depends on Copper's internal scheduling #187

Open
gbin opened this issue Dec 29, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@gbin
Copy link
Collaborator

gbin commented Dec 29, 2024

For example:
type Input = input_msg!('cl, ImageRGBU8Msg, ImageRGBU8Msg, ImageGrayU8Msg); or
type Input = input_msg!('cl, ImageRGBU8Msg, ImageGrayU8Msg, ImageRGBU8Msg); for rerun is not obvious at first glance from:

(
    tasks: [
        (
            id: "cam",
            type: "copper::tasks::webcam::Webcam",
            config: {
                "camera_id": 0,
                "res_rows": 480,
                "res_cols": 640,
                "fps": 30,
            },
        ),
        (
            id: "cam_garden",
            type: "copper::tasks::rtsp::RtspCamera",
            config: {
                "url": "rtsp://tapo_entrance:[email protected]:554/stream2",
            },
        ),
        (
            id: "rerun",
            type: "copper::tasks::rerun_viz::RerunViz",
        ),
        (
            id: "sobel",
            type: "copper::tasks::sobel::Sobel",
        ),
    ],
    cnx: [
        (src: "cam_garden", dst: "rerun", msg: "copper::tasks::ImageRGBU8Msg"),
        (src: "cam", dst: "rerun", msg: "copper::tasks::ImageRGBU8Msg"),
        (src: "sobel", dst: "rerun", msg: "copper::tasks::ImageGrayU8Msg"),
        (src: "cam", dst: "sobel", msg: "copper::tasks::ImageRGBU8Msg"),
    ],
    logging: (
        slab_size_mib: 1024, // Preallocates 1GiB of memory map file at a time
        section_size_mib: 100, // Preallocates 100MiB of memory map per section for the main logger.
    ),
)
@gbin gbin added the bug Something isn't working label Dec 31, 2024
@hscoelho
Copy link
Contributor

hscoelho commented Feb 2, 2025

I can work on this but I want to clarify what the fix should be.
From a quick investigation and some tests, it appears like the order of the inputs is related to the tasks array in the config. I think this is the relevant code:

https://github.com/copper-project/copper-rs/blob/3941df577b9b2ce865c8c0ed3b0c1f9c240425a7/core/cu29_runtime/src/curuntime.rs#L324C1-L327C63

Before I investigate any further, I wanted to know if changing this behavior to sort by the order the inputs appear in the cnx array would be enough to close this issue? That's the order I would expect naturally(and maybe most users).
Another option would be to make it more obvious by changing how the cnx config is written for sinks that take multiple inputs, maybe by making the src property an array with the correct order.

@gbin
Copy link
Collaborator Author

gbin commented Feb 3, 2025

I think the order of declaration of the cnx should work. I want to be sure that we are covering the use cases so maybe we should start making some unit test set to be sure we don't break something :)

I thought about the second solution but getting a list might be weird because it forces the users to do some kind of array association:
(src: ["cam_garden", "cam", "sobel"], dst: "rerun", msg: ["copper::tasks::ImageRGBU8Msg", "copper::tasks::ImageRGBU8Msg", "copper::tasks::ImageGrayU8Msg"]),

Do you have another syntax in mind?

@hscoelho
Copy link
Contributor

hscoelho commented Feb 3, 2025

That was the first syntax I thought of, but an alternative would be something like:

(src: [
    (id: "cam_garden", msg: "copper::tasks::ImageRGBU8Msg"),
    (id: "cam", msg: ""copper::tasks::ImageRGBU8Msg"),
    (id: "sobel", msg: "copper::tasks::ImageGrayU8Msg")
], dest: "rerun")

Either way, I agree with the unit testing, so I'm going to start with a PR just with the unit test while I think about the proper solution.

@gbin
Copy link
Collaborator Author

gbin commented Feb 3, 2025

Just some food for thoughts for you ...there is a strange asymmetry with the combined syntax:
so this is an n to 1 connection combining sources
but we also have 1 to n, should we allow combining destinations?
but how about a task can be part of both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants