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

make the copper app buildable directly from a CopperContext #135

Open
gbin opened this issue Dec 5, 2024 · 3 comments
Open

make the copper app buildable directly from a CopperContext #135

gbin opened this issue Dec 5, 2024 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@gbin
Copy link
Collaborator

gbin commented Dec 5, 2024

Today we have a helper basic_copper_setup that sets up all the runtime for copper:

    let copper_ctx = basic_copper_setup(&PathBuf::from(logger_path), SLAB_SIZE, false, None)
        .expect("Failed to setup copper.");

But the generated application from the ron file gets separately the clock and the logger from that context.

    let mut application =
        PtCloudsApplication::new(copper_ctx.clock.clone(), copper_ctx.unified_logger.clone())
            .expect("Failed to create application.");

Maybe we could officialise this CopperContext and just have the generated application from cu29_derive get a context?

@gbin gbin added enhancement New feature or request good first issue Good for newcomers labels Dec 5, 2024
@hscoelho
Copy link
Contributor

I'd like to try this one! Just to make sure I understand, after this change, the code that builds the application should be something like this?

    let mut application =
        PtCloudsApplication::new(&copper_ctx)
            .expect("Failed to create application.");

Also, I'm not sure if the idea is to replace the current "new" method, which would break code from this version that use it, or to keep the current "new" method and create another method that takes CopperContext.
I was going to suggest the From trait, so the application could be built with App::From(&context) or context.into(), but I'm not sure it's the optimal solution, because if sim_mode is set to true, the "new" method needs a callback parameter that is not available in the CopperContext.

@gbin
Copy link
Collaborator Author

gbin commented Dec 22, 2024

maybe adding another new_from_context or something like that would be a little bit more flexible & idiomatic. That said we could always add parameters to the context.

Another proposal would be to use a builder pattern like : Application::new().with_context(...).sim_mode(...)... .build() // would be very nice no?

also I think we would need to move context in the cu29_runtime crate to make that cleaner.

Let's get a first draft going and we can jam on it.

Thanks for picking that up!

@hscoelho
Copy link
Contributor

The builder pattern looks like a good solution!
I will experiment with the code and submit a draft when I have something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants