panic for constants

[Frank-Buss]

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.

[Frank-Buss]

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.

[rooooooooob]

@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.

[Frank-Buss]

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.

[rooooooooob]

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.

[Frank-Buss]

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);
}

[rooooooooob]

@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.

[Frank-Buss]

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.

[rooooooooob]

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.

[Frank-Buss]

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.

[Frank-Buss]

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.

[Frank-Buss]

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.

[rooooooooob]

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
]

[Frank-Buss]

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 ).