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

subscribe_json required for (some?) additional outputs instead of direct deserialisation with subscribe_value #1448

Closed
tuxbotix opened this issue Sep 16, 2024 · 4 comments

Comments

@tuxbotix
Copy link
Contributor

tuxbotix commented Sep 16, 2024

I encoutered this in #1447,

I'm subscribing to an Option<calibration::corrections::Corrections> value at Control.additional_outputs.last_calibration_corrections (current main). In my understanding, subscribe_value should work and directly give me the deserialisation.

For example, these seems to be working?

// `tools/twix/src/panels/image_color_select.rs`
let field_color:  BufferHandle<FieldColorParameters>= nao.subscribe_value(format!(
    "parameters.field_color_detection.{cycler_path}",
    cycler_path = cycler.as_snake_case_path()
));

// OR `tools/twix/src/panels/ball_candidates.rs`
let ball_candidates: BufferHandle<Option<Vec<CandidateEvaluation>>> =
    nao.subscribe_value(format!("{cycler_path}.additional_outputs.ball_candidates"));

But it didn't for Corrections! While there were no compilation or runtime errors, the deserialised values were garbage, easily verified when comparing the values in a Text panel.

let calibration_corrections: BufferHandle<Option<Corrections>>=
            nao.subscribe_value("Control.additional_outputs.last_calibration_corrections");

if let Some(value) = calibration_corrections.get_last_value().ok().flatten().flatten() {
    // type of value -> calibration::corrections::Corrections
    println!("{:?}", value);
}

But this works:

let calibration_corrections =
            nao.subscribe_json("Control.additional_outputs.last_calibration_corrections");

if let Some(value) = self
    .calibration_corrections
    .get_last_value()
    .ok()
    .flatten()
    .and_then(|value| serde_json::from_value::<Corrections>(value).ok())
{
    // type of value -> calibration::corrections::Corrections
    // works but ugly AF IMHO
    println!("{:?}", value);
}

So it looks like (some?) additional outputs are sent as JSON? or what's going on O.o , is there something wrong with the type itself?

@schmidma
Copy link
Member

Just had a brief look, but do I see it correctly, that you subscribe to an Option<Option<T>> and not an Option<T>. If so, you have to specify the correct type (double wrapped Option). JSON is able to flatten this nesting, bincode in the current implementation is not.

@tuxbotix
Copy link
Contributor Author

tuxbotix commented Sep 20, 2024

Huh I'm a bit confused now, the behaviour you explained seems to be the case, but I assumed the semantics differently:

The node has below additional output:

last_calibration_corrections:
        AdditionalOutput<Option<Corrections>, "last_calibration_corrections">,

so I assumed that I need to have a BufferHandle<Option<Corrections>> to subscribe. Am I mistaken? or should it be BufferHandle<Corrections> instead?

@schmidma
Copy link
Member

It should be BufferHandle<Option<Option<Corrections>>>, as AdditionalOutput itself already introduces a layer of "optionality". As it is an additional output the node may decide not to produce the output, which in turn is reflected in having a None as output.

IIRC, there is the missing feature (some might call it a bug) that new cycles do not reset additional outputs to Nones. But that might be a different topic

@tuxbotix
Copy link
Contributor Author

Aha now I understand what's going on! I'll change the node to remove the extra Option and see how that works out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants