Skip to content

panic for constants #241

Open
Open
panic for constants#241

Description

@Frank-Buss
Frank-Buss
opened on Oct 30, 2024

I have this CDDL file:

cmd_start = 0
cmd_stop = 1

my_command = [
    command: cmd_start/cmd_stop
]

This works with zcbor.py, but with cddl-codegen, I get this output:

thread 'main' panicked at src/intermediate.rs:1748:31:
should not expose Fixed type in member, only needed for serializaiton: Fixed(Uint(0))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I think it is valid CDDL, but maybe it is some edge case. Would be also ok if there is a way to write it differently, and if this works then with zcbor as well.

Activity

  1. Frank-Buss commented on Oct 31, 2024

    @Frank-Buss
    Frank-Buss
    on Oct 31, 2024
    Author

    I tried this patch:

    --- a/src/intermediate.rs
+++ b/src/intermediate.rs
@@ -1612,7 +1612,7 @@ impl ConceptualRustType {
     /// Function parameter TYPE by-non-mut-reference for read-only
     pub fn _for_rust_read(&self, types: &IntermediateTypes, cli: &Cli) -> String {
         match self {
-            Self::Fixed(_) => panic!(
+            Self::Fixed(_) => format!(
                 "should not expose Fixed type, only here for serialization: {:?}",
                 self
             ),
@@ -1745,9 +1745,8 @@ impl ConceptualRustType {
     /// Type when storing a value inside of a rust struct. This is the underlying raw representation.
     pub fn for_rust_member(&self, types: &IntermediateTypes, from_wasm: bool, cli: &Cli) -> String {
         match self {
-            Self::Fixed(_) => panic!(
-                "should not expose Fixed type in member, only needed for serializaiton: {:?}",
-                self
+            Self::Fixed(_) => format!(
+                "u8"
             ),
             Self::Primitive(p) => p.to_string(),
             Self::Rust(ident) => {

    It creates 2 unnecessary types, but it compiles, and the serialization code looks good, but haven't tested it yet. Of course, this would be an ugly solution, it should just ignore it for constants and not trying to create types for it, but haven't looked at the rest of the code how to do this.

  2. rooooooooob commented on Oct 31, 2024

    @rooooooooob
    rooooooooob
    on Oct 31, 2024
    Contributor

    @Frank-Buss

    try:

    my_command = [
   ; @name cmd_start
   0 //
   ; @name cmd_stop
   1
]

    if that format works for you.

    What are the unnecessary types? Did you create 2 extra cmd_start / cmd_stop types to get it to work? I guess it would depend on how you re-wrote the cddl.

    In case you're talking about a MyCommandKind, it's because for WASM we create a EnumKind for each Enum type because group choices have/can contain extra information e.g. the array length and we code this for the general case e.g. group_choice = [0, uint // tstr, bytes]. You cannot expose these directly to wasm and thus we generate the EnumKind type to let wasm_bindgen have a way of easily differentiating types (no pattern matching like in rust). This is more important for your case when running with --preserve-encodings=true as then the group choice has to keep information about its encoding (definite vs indefinite array encoding, and if definite, how many bytes were used for the length part or if it was inlined) to ensure correct round-tripping and exposing control over encoding details. In the protocols we work with when it is a genuine C-style enum we've usually seen it as a type choice instead (no need to waste extra byte(s) on the array part):

    my_command = 0 ; @name cmd_start
           / 1 ; @name cmd_stop

    which wouldn't generate the extra MyCommandKind type in rust. This won't help if you're interfacing with an existing protocol though.

  3. Frank-Buss commented on Oct 31, 2024

    @Frank-Buss
    Frank-Buss
    on Oct 31, 2024
    Author

    Thanks, but with the @ comments doesn't work with zcbor, and I want to use the same CDDL files for both, the C program (a Zephyr project) and the Rust project. With my patch it works. It generates an extra type, but also an enum. Here is the generated lib.rs for the CDDL file I posted:

    #![allow(clippy::too_many_arguments)]

pub mod error;
// This file was code-generated using an experimental CDDL to rust tool:
// https://github.com/dcSpark/cddl-codegen

pub mod serialization;

use crate::error::*;
use std::collections::BTreeMap;
use std::convert::TryFrom;

pub type CmdStart = u8;

#[derive(Copy, Eq, PartialEq, Ord, PartialOrd, Clone, Debug)]
#[wasm_bindgen::prelude::wasm_bindgen]
pub enum CmdStartOrCmdStop {
    CmdStart,
    CmdStop,
}

pub type CmdStop = u8;

#[derive(Clone, Debug)]
pub struct MyCommand {
    pub command: CmdStartOrCmdStop,
}

impl MyCommand {
    pub fn new(command: CmdStartOrCmdStop) -> Self {
        Self { command }
    }
}

    I think it is a bug in cddl-codegen that it tries to generate a type for a number constant. And I think it should be valid CDDL, at least syntactically: According to the ABNF of RFC 8610, a rule is a typename = type, where type can be type1, which can be type2, which can be a value, which can be a number.

  4. rooooooooob commented on Oct 31, 2024

    @rooooooooob
    rooooooooob
    on Oct 31, 2024
    Contributor

    the @ comments doesn't work with zcbor

    Yeah the ; @<tag> notation is a cddl-codegen specific thing we've done to provide extra information when it's not directly available e.g. to tell cddl-codegen to encode it differently or to derive rust traits or to rename things. You could just remove it but then your types will be called like MyCommand::I0 and MyCommand::I1

    I think it should be valid CDDL, at least syntactically

    It is.

    I think it is a bug in cddl-codegen that it tries to generate a type for a number constant.

    It probably is. I thought we had constant support although for wasm it does it via function like function cmd_start(): number.

  5. Frank-Buss commented on Nov 6, 2024

    @Frank-Buss
    Frank-Buss
    on Nov 6, 2024
    Author

    Might be a related problems with constants, haven't looked into it, but it currently blocks my project. This file:

    type1 = 1
type2 = 2

foo = (
    type: type1,
    something1: uint .size 4
)

bar = (
    type: type2
    something2: uint .size 4
    something3: uint .size 4
)

foobar = [
    foo/bar
]

    results in this crash (with my applied patch, otherwise it crashes already for the constants earlier) :

    thread 'main' panicked at src/intermediate.rs:1408:64:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

    Problem is the last foobar definition. If I remove it, then it works, but it is not usable for me. I guess this is a good candidate for a new test case? I simplified it already from my real usecase.

    zcbor works just fine with it. Calling it like this:

    zcbor code -c test.cddl -d -t foobar --oc test.c --oh test.h --time-header --copy-sources

    generates this file:

    /*
 * Generated using zcbor version 0.9.0
 * https://github.com/NordicSemiconductor/zcbor
 * at: 2024-11-06 16:34:17
 * Generated with a --default-max-qty of 3
 */

#include <stdint.h>
#include <stdbool.h>
#include <stddef.h>
#include <string.h>
#include "zcbor_decode.h"
#include "test.h"
#include "zcbor_print.h"

#if DEFAULT_MAX_QTY != 3
#error "The type file was generated with a different default_max_qty than this file"
#endif

#define log_result(state, result, func) do { \
        if (!result) { \
                zcbor_trace_file(state); \
                zcbor_log("%s error: %s\r\n", func, zcbor_error_str(zcbor_peek_error(state))); \
        } else { \
                zcbor_log("%s success\r\n", func); \
        } \
} while(0)

static bool decode_foo(zcbor_state_t *state, struct foo *result);
static bool decode_bar(zcbor_state_t *state, struct bar *result);
static bool decode_foobar(zcbor_state_t *state, struct foobar *result);


static bool decode_foo(
                zcbor_state_t *state, struct foo *result)
{
        zcbor_log("%s\r\n", __func__);

        bool res = (((((zcbor_uint32_expect(state, (1))))
        && ((zcbor_uint32_decode(state, (&(*result).foo_something1)))
        && ((((*result).foo_something1 <= UINT32_MAX)) || (zcbor_error(state, ZCBOR_ERR_WRONG_RANGE), false))))));

        log_result(state, res, __func__);
        return res;
}

static bool decode_bar(
                zcbor_state_t *state, struct bar *result)
{
        zcbor_log("%s\r\n", __func__);

        bool res = (((((zcbor_uint32_expect(state, (2))))
        && ((zcbor_uint32_decode(state, (&(*result).bar_something2)))
        && ((((*result).bar_something2 <= UINT32_MAX)) || (zcbor_error(state, ZCBOR_ERR_WRONG_RANGE), false)))
        && ((zcbor_uint32_decode(state, (&(*result).bar_something3)))
        && ((((*result).bar_something3 <= UINT32_MAX)) || (zcbor_error(state, ZCBOR_ERR_WRONG_RANGE), false))))));

        log_result(state, res, __func__);
        return res;
}

static bool decode_foobar(
                zcbor_state_t *state, struct foobar *result)
{
        zcbor_log("%s\r\n", __func__);
        bool int_res;

        bool res = (((zcbor_list_start_decode(state) && ((((zcbor_union_start_code(state) && (int_res = ((((decode_foo(state, (&(*result).foobar_union_foo_m)))) && (((*result).foobar_union_choice = foobar_union_foo_m_c), true))
        || (zcbor_union_elem_code(state) && (((decode_bar(state, (&(*result).foobar_union_bar_m)))) && (((*result).foobar_union_choice = foobar_union_bar_m_c), true)))), zcbor_union_end_code(state), int_res)))) || (zcbor_list_map_end_force_decode(state), false)) && zcbor_list_end_decode(state))));

        log_result(state, res, __func__);
        return res;
}



int cbor_decode_foobar(
                const uint8_t *payload, size_t payload_len,
                struct foobar *result,
                size_t *payload_len_out)
{
        zcbor_state_t states[4];

        return zcbor_entry_function(payload, payload_len, (void *)result, payload_len_out, states,
                (zcbor_decoder_t *)decode_foobar, sizeof(states) / sizeof(zcbor_state_t), 1);
}

  6. rooooooooob commented on Nov 6, 2024

    @rooooooooob
    rooooooooob
    on Nov 6, 2024
    Contributor

    @Frank-Buss

    foobar = [
    foo/bar
]

    I'm not sure this is technically proper CBOR as both are basic groups still and not yet concrete types. Try doing foo // bar instead if you want to use group choices (//). Type choices (/) require the type variants to be concrete types afaik, but maybe all type choices being a basic group is enough to lock in the type, but type choices as per the CDDL RFC are meant to be used on types and a basic group isn't a type until it knows if it's an array or map group as that completely changes the CBOR representation. Either way I can't imagine what [foo / bar] would represent that wouldn't just be [foo // bar] so try that instead, as the second notation is meant specifically for this situation. I can't find anything 100% in the RFC but it is explicit about group choices being used for groups and type choices on types and that groups on their own don't seem to be types yet, unless made into one by wrapping it with []/{}, which in this case would happen after the point where you need it to be a type. I could be wrong here but it just seems like using group choices instead is what you would want here. Basic groups (like foo/bar here defined with only (/)) don't have a concrete cbor definition yet (they are abstract concepts since you don't know if they're array or map yet but maybe zbor just assumes based on the context there), to do that you need to do e.g.

    baz = [
  a: uint,
  foo, ; this embeds all fields into baz so baz has 3 fields
]

    or

    baz = [
  a: uint,
  foo: [foo], ; now baz has 2 fields, the second of which is an array with 2 elements aka foo
]

    or group choices

    foobar = [
    foo // bar
]

    We use a lot of similar cddl defs written like:

    ; probably not as useful for you since you can't use our @name notation to name the variants
foobar = [
    type: 0, something1: uint .size 4 //
    type: 1, something2: uint .size 4, something3: uint .size 4
]

    or

    foo = (
    type: 1,
    something1: uint .size 4
)

bar = (
    type: 2
    something2: uint .size 4
    something3: uint .size 4
)

foobar = [
    foo // bar
]

    I hope this helps.

  7. Frank-Buss commented on Nov 6, 2024

    @Frank-Buss
    Frank-Buss
    on Nov 6, 2024
    Author

    Thanks, looks like zcbor generates the same code for "//". But I need a comma at the end of the lines:

    type1 = 1
type2 = 2

foo = (
    type: type1,
    something1: uint .size 4
)

bar = (
    type: type2,
    something2: uint .size 4,
    something3: uint .size 4
)

foobar = [
    foo//bar
]

    cddl-codegen works without commas as well (with my patch with the constants). Haven't checked the BNF, is a comma required?

    Will check if the Rust code works tomorrow.

  8. rooooooooob commented on Nov 7, 2024

    @rooooooooob
    rooooooooob
    on Nov 7, 2024
    Contributor

    Commas work. Sorry, that was a mistake on my end, I simply forgot to add them there since I was only typing in the browser as an example and had copied/pasted from other lines. I'm surprised it worked without commas, I would have thought they would be required based on the CDDL RFC. I didn't write the parsing code though, we use the external cddl crate for that and then just operate on their AST to do our codegen.

  9. Frank-Buss commented on Nov 7, 2024

    @Frank-Buss
    Frank-Buss
    on Nov 7, 2024
    Author

    I tested it, and there is one compilation error for the generated code. Here is the test file:

    TYPE1 = 0
TYPE2 = 1
TYPE3 = 2

FOO = 0
BAR = 1
BAZ = 2

group_baz = (
   type_id: BAZ,
   time1:   uint .size 8,
   time2:   uint .size 8,
   ? max:   uint .size 4
   )

group_foo_or_bar = (
   type_id: FOO/BAR
)

beta = [
   TYPE1,
   group_baz//group_foo_or_bar
]

    For this, it generates this code in serialization.rs:

    impl cbor_event::se::Serialize for Beta {
    fn serialize<'se, W: Write>(
        &self,
        serializer: &'se mut Serializer<W>,
    ) -> cbor_event::Result<&'se mut Serializer<W>> {
        match self {
            Beta::Beta0(group_baz) => {
                serializer.write_array(cbor_event::Len::Len(
                    1 + 3
                        + match &self.group_baz.max {
                            Some(x) => 1,
                            None => 0,
                        },
                ))?;
                serializer.write_unsigned_integer(0u64)?;
                group_baz.serialize_as_embedded_group(serializer)?;
                Ok(serializer)
            }
            Beta::GroupFooOrBar(group_foo_or_bar) => group_foo_or_bar.serialize(serializer),
        }
    }
}

    The error is in line "+ match &self.group_baz.max {". It needs to be "+ match &group_baz.max {". This is probably easy to fix, but couldn't find it.

    Haven't tested yet if it runs, but at least now it compiles everything.

  10. Frank-Buss commented on Nov 7, 2024

    @Frank-Buss
    Frank-Buss
    on Nov 7, 2024
    Author

    Looks like the deserialize had a tricky problem: It did a read_len.read_elems(4), independent of the type. But with a group_foo_or_bar, the array has only 1 element. I also fixed this manually, looks like now I can read it correctly.

    Might be good to add my last test as a new test case and then enhance the tests to try to serialize and deserialize it.

  11. Frank-Buss commented on Nov 7, 2024

    @Frank-Buss
    Frank-Buss
    on Nov 7, 2024
    Author

    Another problem: The Rust code expected an additional array for the inner group_baz//group_foo_or_bar. But the zcbor code generated it inline, without an additional array. I think zcbor is right. After manually fixing this, both types work now for my application. But would be good, if it could be fixed for the cddl-codegen project as well, because I might need to regenerate the code when the CDDL changes, and then I would need to apply the manual patches again.

  12. rooooooooob commented on Nov 7, 2024

    @rooooooooob
    rooooooooob
    on Nov 7, 2024
    Contributor

    I've only ever seen group choices at the full group level e.g. [group1 // group2] for basic (non-array/map) groups group1/group2 so the code kind of assumes that structure. If you do [uint, group1 // group2, tstr] I have no idea what the code would generate or even think it is.

    What happens if you do this? e.g. move the TYPE1 into the other two groups.

    group_baz = (
   TYPE1,
   type_id: BAZ,
   time1:   uint .size 8,
   time2:   uint .size 8,
   ? max:   uint .size 4
   )

group_foo_or_bar = (
   TYPE1,
   type_id: FOO/BAR
)

beta = [
   group_baz//group_foo_or_bar
]

  13. Frank-Buss commented on Nov 8, 2024

    @Frank-Buss
    Frank-Buss
    on Nov 8, 2024
    Author

    I'll test it, but is my example valid CDDL, and which program is right, zcbor or cddl-codegen? But I think this is a different problem. I think I fixed the constant problem, see here: #242 and we can close this issue, and create a new issue for the nested array, if it is wrong, otherwise we should create an issue in the zcbor repository ( https://github.com/NordicSemiconductor/zcbor ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions