Consider placing `data` in a separate repository for consumption by other libraries

[johnlettman]

Hello!

I am working on a Rust no-std implementation of OpenPrintTag that consumes the YAML data to avoid duplication of efforts and to keep your upstream as a single source of truth.

Currently, the YAML data definitions are provided in the data sub-directory of this repository, which makes it trickier to pull this repository and extract or scrape the data from the GitHub tree.

It would be far more trivial for me and others to pull from a repository dedicated to the definitions. Furthermore, it has the added benefit of decoupling it from the Python implementation and allowing it to be versioned independently -- perhaps with workflows to produce releases -- and pull requests would be more contextually relevant as they wouldn't overlap with PRs to the Python code.

[yanshay]

Only out of curiosity, what makes it trickier to fetch the yaml files from the data folder?

I agree if they plan a repo per language then the 'data' folder should be a separate repo as well, but it may end up being a single repo for all languages with folder per language, and then the definition would be in one of the folders.

I think there is also formal data definition language that could be used - CDDL (sort of schema for CBOR), which may also make sense to add to this project. Maybe there are already generators for code from such definitions.

[johnlettman]

@yanshay

Only out of curiosity, what makes it trickier to fetch the yaml files from the data folder?

I wouldn't say it's a huge burden. In my case, it seems the options are:

1. Pull this repo and extract the data portion from the rest of the library (pulling extra unnecessary files), or
2. Copy the data portion into my library (no automated method of updating unless implementing 1.)

It would just be "nicer" as a separate repo so you're only pulling the definitions, not anything else. Separate tracking and versioning for the definitions is an added bonus in my eyes.

I agree if they plan a repo per language then the 'data' folder should be a separate repo as well

There is also the case of outside developers contributing with all sorts of languages, libraries, and applications -- not necessarily just the @prusa3d org. In such a case, it is cleaner for those developers while holding the door open for expansion on the Prusa side.

I think there is also formal data definition language that could be used - CDDL (sort of schema for CBOR), which may also make sense to add to this project. Maybe there are already generators for code from such definitions.

Absolutely! I agree. It actually seems as if the data portion of this project is essentially doing just that. I have had success building a schema from their YAML definitions and then generating Rust code by building up a syntax tree from their "schema." It happens at build time rather than run time because I am targeting embedded systems that cannot handle a YAML parser. In a way, it is a code generator.

I reckon there's plenty of descriptiveness in their definitions to accomplish this fully.

[yanshay]

You could also pull the files over http. I think by naming conventions they point one to another so if you start with the main you could even pull them all without knowing the entire folder content in advance.

When you have it working for embedded rust I'd be happy to try it out.

[CZDanol]

Hello :)

We're happy to see such interest in the OPT project.

Regarding the directory placement: I don't see a problem with having the whole OPT repo as a git submodule/subrepo (which is imo better than just copying the yamls, because this gives you better version control and ability to pull from the upstream). We're doing it with plenty of libraries in the Buddy project: https://github.com/prusa3d/Prusa-Firmware-Buddy/tree/master/lib.

Also, your project will likely benefit for example from the test suite as well, that you could possibly reuse to check your implementation.

Speaking of the test suite, I observe that we're starting to get quite a number of the OPT implementations now, and from brief looks on the code, the implementation is not up to the spec. So when implementing your library, please try to read the spec carefully and consider all the aspects.

Notably:

• Just reading the aux_region_offset is not enough, for proper implementation, you need to consider all the fields in the meta region.
• The spec clearly states that implementations must support unknown keys (that don't even have to be integer types) and preserve them on data updating

I will try to extend the test suite in the repo to cover these edge cases, so that programmers have more resources for implementing/checking this than just the specification text.

[CZDanol]

Oh, also, we've released the C++ library now, feel free to take inspiration: https://github.com/prusa3d/Prusa-Firmware-Buddy/tree/openprinttag/src/module/nfc/openprinttag

[yanshay]

It's great that you chose to use CBOR!

I Rust it's mostly about defining the structure and everything else is automated by other crates (except the Meta offsets which is a single if). Including missing fields, etc.

So for a full implementation it's just converting the yaml to a struct.

I was able to parse the data I needed in really no time.

[CZDanol]

Glad you approve :) But please don't forget about the unknown fields. And the meta offsets are not a single if. The specification allows for example to have the aux region before the main region, and that could be defined only for example by setting main_region_offset: 100, aux_region_offset: 10. In that case, you need to be able to correctly determine the size of the aux region from main_region_offset.

[yanshay]

Glad you approve :) But please don't forget about the unknown fields. And the meta offsets are not a single if. The specification allows for example to have the aux region before the main region, and that could be defined only for example by setting main_region_offset: 100, aux_region_offset: 10. In that case, you need to be able to correctly determine the size of the aux region from main_region_offset.

Yes, that's what I referred to by the 'if' above.
Since I personally now only interested in the main I just 'if' there is an offset to main or not after separately reading the meta.

[johnlettman]

RE @yanshay

You could also pull the files over http.

I implemented this today.

It does seem to be a fairly reliable way to get the most recent main version of those schemas; however, one thing to note is that a locally checked-out or submoduled copy would be a better approach for the generated Rust core of my library, e.g.,

println!("cargo:rerun-if-changed=data/");

Obviously, I understand that accommodating every implementation is an impossible request, so it's no sweat if ultimately separating the contexts is too big of an ask, I can certainly work with whatever you decide. :)

[yanshay]

Two questions/comments on the Rust code:

1. I see for no_std you used heapless. Worth adding an alloc feature for using Vec in no_std with alloc. I use it all the time and for me it's almost mandatory for compatibility with the rest of the code.
2. I see there the rust structs/enum skeleton are already generated. What do you plan to use for the CBOR? I used I minicbor with minicbor-derive in my tests, which you can add to your generated code pretty easily. Just need to auto generate the attributes and it will all be ready to use.

You could still beat the javascript implementation timeline with a rust version 😄

[johnlettman]

1. The idea is that it'll be feature-gated through the std feature. When std is selected, the EnumArray<T, N> will alias to Vec<T>. Otherwise, the upper bound on the fixed-size heapless::Vec is defined in N. Since the max size of the tags list is a known quantity, we can safely lean on that at the expense of stacking, well, the full size. I haven't considered whether I'd create something in the middle yet.
2. I intended on going for minicbor and implementing generated decode functions that will pull apart the (key => field) maps and assemble the structs in one go. I see now that the _derive crate can do that, which may simplify things a lot!

Of note, I do plan on spreading out to C FFI, WASM, and maybe pyo3 if I'm feeling brave enough.

EDIT: I will say a lot of the architecture is there to support the manual function approach 😅

[yanshay]

1. The idea is that it'll be feature-gated through the std feature. When std is selected, the EnumArray<T, N> will alias to Vec<T>. Otherwise, the upper bound on the fixed-size heapless::Vec is defined in N. Since the max size of the tags list is a known quantity, we can safely lean on that at the expense of stacking, well, the full size. I haven't considered whether I'd create something in the middle yet.

Just that if you use std:Vec it will not work on no_std with alloc because std is not available. So need a feature flag to use alloc::Vec instead of std::Vec. That's why crates supporting no_std many times have an alloc feature flag when they want to support Vec or String or Box or ...

3. I intended on going for minicbor and implementing generated decode functions that will pull apart the (key => field) maps and assemble the structs in one go. I see now that the _derive crate can do that, which may simplify things a lot!>

Yes, the _derive is probably a few extra lines of code to your code generation to be complete.
Here is the definition I came up with (which is enough for my needs at this time), might save you a bit to see the structure:

#[derive(Debug, Encode, Decode)]
#[cbor(map)]  
struct Meta {
    #[n(0)] main_region_offset: Option<usize>,
    #[n(1)] main_region_size: Option<usize>, 
    #[n(2)] aux_region_offset: Option<usize>,
    #[n(3)] aux_region_size: Option<usize>, 
}

#[derive(Debug, Encode, Decode)]
#[cbor(map)]  
struct MainRegion {
    #[n(10)] material_name: Option<String>, // Color Name?
    #[n(11)] brand_name: Option<String>,
    #[n(52)] material_abbreviation: Option<String>,
    #[n(9)] material_type: Option<MaterialType>,
    #[n(16)] nominal_netto_full_weight: Option<usize>, // label
    #[n(17)] actual_netto_full_weight: Option<usize>, // real weight at start
    #[n(18)] empty_container_weight: Option<usize>, // empty 
    #[cbor(n(19), with = "minicbor::bytes")] primary_color: Option<Vec<u8>>,
    #[cbor(n(20), with = "minicbor::bytes")] secondary_color_0: Option<Vec<u8>>,
    #[cbor(n(21), with = "minicbor::bytes")] secondary_color_1: Option<Vec<u8>>,
    #[cbor(n(22), with = "minicbor::bytes")] secondary_color_2: Option<Vec<u8>>,
    #[cbor(n(23), with = "minicbor::bytes")] secondary_color_3: Option<Vec<u8>>,
    #[cbor(n(24), with = "minicbor::bytes")] secondary_color_4: Option<Vec<u8>>,
}

#[derive(Debug, Encode, Decode)]
#[cbor(index_only)]  
pub enum MaterialType {
    #[n(0)] PLA,
    #[n(1)] PETG,
    #[n(2)] TPU,
    #[n(3)] ABS,
    #[n(4)] ASA,
    #[n(5)] PC,
    #[n(6)] PCTG,
    #[n(7)] PP,
    #[n(8)] PA6,
    #[n(9)] PA11,
    #[n(10)] PA12,
    #[n(11)] PA66,
    #[n(12)] CPE,
    #[n(13)] TPE,
    #[n(14)] HIPS,
    #[n(15)] PHA,
    #[n(16)] PET,
    #[n(17)] PEI,
    #[n(18)] PBT,
    #[n(19)] PVB,
    #[n(20)] PVA,
    #[n(21)] PEKK,
    #[n(22)] PEEK,
    #[n(23)] BVOH,
    #[n(24)] TPC,
    #[n(25)] PPS,
    #[n(26)] PPSU,
    #[n(27)] PVC,
    #[n(28)] PEBA,
    #[n(29)] PVDF,
    #[n(30)] PPA,
    #[n(31)] PCL,
    #[n(32)] PES,
    #[n(33)] PMMA,
    #[n(34)] POM,
    #[n(35)] PPE,
    #[n(36)] PS,
    #[n(37)] PSU,
    #[n(38)] TPI,
    #[n(39)] SBS,
}

Of note, I do plan on spreading out to C FFI, WASM, and maybe pyo3 if I'm feeling brave enough.

EDIT: I will say a lot of the architecture is there to support the manual function approach 😅

[johnlettman]

Just that if you use std:Vec it will not work on no_std with alloc because std is not available. So need a feature flag to use alloc::Vec instead of std::Vec. That's why crates supporting no_std many times have an alloc feature flag when they want to support Vec or String or Box or ..

Oh, absolutely. Since I am building my own tree of schema definitions, I have a sense of the "at most" or, in some cases, max_length values for any list.

In the generated core, this looks like:

#[cfg(feature = "std")]
#[doc = "An array of enum values.\n"]
pub type EnumArray<T, const N: usize> = std::vec::Vec<T>;

#[cfg(not(feature = "std"))]
#[doc = "An array of enum values.\n"]
pub type EnumArray<T, const N: usize> = heapless::Vec<T, N>;

Since nobody should be directly touching EnumArray, the dangling type var shouldn't matter.

The idea is to enable pure nostd as much as possible for embedded environments. I want to have first-class support for nostd, with std stuff as an added bonus.

I haven't reviewed the access semantics yet, but those could be easily unified with a common trait.

Yes, the _derive is probably a few extra lines of code to your code generation to be complete. Here is the definition I came up with (which is enough for my needs at this time), might save you a bit to see the structure: [...]

I highly agree. It is a game-changer. I think the reason I started out on my approach was to have significant control over the deserialization phase (and being unaware of the macros for _derive). Depending on the overhead, I might still do that, but _derive is far more attractive if I can get everything I'd like from it.

I mostly wrote it the way I did to avoid ever typing the values from the data schema here and using them to generate everything. There is likely still some cruft from evolving the design to accommodate that - e.g., the crazy contextual deserialization needed to associate enum -> discriminators because of how the data schema is structured.

[yanshay]

In the generated core, this looks like:

#[cfg(feature = "std")]
#[doc = "An array of enum values.\n"]
pub type EnumArray<T, const N: usize> = std::vec::Vec<T>;

#[cfg(not(feature = "std"))]
#[doc = "An array of enum values.\n"]
pub type EnumArray<T, const N: usize> = heapless::Vec<T, N>;

That's what I referred to.

My no_std app on ESP32 (with esp-hal) can't work with neither options you provided there.

I'd need an addition something like:

#[cfg(feature = "alloc")]
pub type EnumArray<T, const N: usize> = alloc::vec::Vec<T>;

(or it can be simplified to use use alloc::vec::Vec vs. use std::vec::Vec depending on feature flags and then the rest can be the same just based on Vec.

[johnlettman]

That's what I referred to.

My no_std app on ESP32 (with esp-hal) can't work with neither options you provided there.

I'd need an addition something like:

#[cfg(feature = "alloc")]
pub type EnumArray<T, const N: usize> = alloc::vec::Vec<T>;

(or it can be simplified to use use alloc::vec::Vec vs. use std::vec::Vec depending on feature flags and then the rest can be the same just based on Vec.

Oh! Okay, I think I understand now.
I can't just be between full std and nostd (e.g., like WASM) if I wanted to capture your use case, since you have alloc available.

We're probably looking at a final type like this:

#[cfg(feature = "std")]
pub type EnumArray<T, const N: usize> = std::vec::Vec<T>;

#[cfg(all(not(feature = "std"), feature = "alloc"))]
pub type EnumArray<T, const N: usize> = alloc::vec::Vec<T>;

#[cfg(all(not(feature = "std"), not(feature = "alloc")))]
pub type EnumArray<T, const N: usize> = heapless::Vec<T, N>;

Sorry for the misunderstanding 😅
I will absolutely work to support all three.