NordicSemiconductor / zcbor

Low footprint C/C++ CBOR library and Python tool providing code generation from CDDL descriptions.
Apache License 2.0
114 stars 34 forks source link

Invalid CDDL or RFC Interpretation Issues? #347

Open nslowell opened 1 year ago

nslowell commented 1 year ago

I have some CDDL that successfully runs through zcbor, but I have now been trying to use cddl-rs (https://docs.rs/cddl/latest/cddl/ and https://github.com/anweiss/cddl) which uses cddl-codegen (https://github.com/dcSpark/cddl-codegen) and have been encountering errors with both my CDDL and some zcbor's test case files.

They are claiming here (https://github.com/dcSpark/cddl-codegen/issues/201) and here (https://github.com/anweiss/cddl/issues/199) that some of it might be invalid CDDL. I'm not attempting to start any kind of battle, but I am wondering if there are some interpretation differences of the RFC that is causing confusion. I was hoping someone could explain why the CDDL test cases are considered valid by zcbor.

Example:

You're using a lot of (key: value) syntax that is confusing the parser. Anything in the form of key: value is a group entry but you're trying to use that syntax to describe types. For example, the #6.10(boolval: bool) is invalid CDDL because the boolval: bool tag type you've specified is not actually a type but rather a group entry

oyvindronningstad commented 1 year ago

They are right that it is invalid CDDL. zcbor's understanding of labels/keys is flawed, but "fixing" it would be more complicated and less useful/flexible than the way it is now, so for the time being it hasn't been fixed. I'm basically still deciding how to proceed.

To briefly describe the difference: zcbor treats labels as separate from map keys. This means you can label a key:value pair in a map. Conversely, the CDDL spec doesn't really have labels, rather it allows both list members and map members to have keys, it's just that in lists, they are ignored in the data, i.e. they can be used as labels. This means you basically can't label anything in maps because having two nested keys is not ok. It also means you can't label anything (like a group or a choice) that already contains a label for one of its members.

; Invalid
Ex1_data = (data_point: bstr)
Ex1 = [
    data: *Ex1_data,
    metadata: bstr,
]

; Valid
data = *(data_point: bstr)
Ex1 = [
    data,
    metadata: bstr,
]

; Invalid
Ex2_Invalid = {
    full_name: 1 => tstr,
    address: 2 => tstr,
}

; Valid
full_name = 1
address = 2
Ex2_Valid = {
    full_name => tstr,
    address => tstr,
}

It also results in some weird cases where if you create a group where members have keys, the keys will be labels if placed in a list, but actual keys if placed in a map.

Ex3 = (
    foo: 1,
    bar: 2,
)

; Here, foo and bar are ignored in the data:
Ex3_List = [Ex3]

; Here, foo and bar are tstr keys:
Ex3_Map = {Ex3}

I welcome anyone's opinions on how you use zcbor, and whether you prefer correctness, or keeping the current state. Though I suspect zcbor will eventually move to be more in line with the CDDL spec.

jnz86 commented 5 months ago

Though I suspect zcbor will eventually move to be more in line with the CDDL spec.

If it isn't to spec, and it's going to have to change, the second best time to do it is now. No point in putting it off. This is what forks and previous commits are for.