hecatia-elegua / bilge

Use bitsized types as if they were a feature of rust.
Apache License 2.0
172 stars 16 forks source link

How to set the default value of a field? Or maybe a feature request for fixed-value fields? #90

Open DavidAntliff opened 4 months ago

DavidAntliff commented 4 months ago

Apologies if I've missed this. I see some discussions on the Default and DefaultBits derive macros, as well as some examples in the docs about default enum values, but what I'm having trouble with is understanding how to set the default value of a field.

For example, I have a struct that represents a hardware instruction on some device:

    #[bitsize(128)]
    #[derive(DebugBits, FromBits)]
    struct AddInstruction {
        src1: u4,
        reserved: u22,
        opcode: u6,
        src2: u4,
        reserved: u28,
        dst: u4,
        reserved: u60,
    }

I want the opcode field to be fixed in this struct. Say, hard-coded to the value "8". Other instructions would have unique structs with their own opcode values.

I've tried using derivation and educe but with little or no success. E.g.

    const ADD_OPCODE: u6 = u6::new(8);

    #[bitsize(128)]
    #[derive(DebugBits, FromBits)]
    #[derive(Educe)]
    #[educe(Default)]
    struct AddInstruction {
        src1: u4,
        reserved: u22,
        #[educe(Default = ADD_OPCODE)]
        opcode: u6,
        src2: u4,
        reserved: u28,
        dst: u4,
        reserved: u60,
    }
error: cannot find attribute `educe` in this scope
  --> src/main.rs:30:11
   |
30 |         #[educe(Default = ADD_OPCODE)]
   |           ^^^^^
   |
   = note: `educe` is in scope, but it is a crate, not an attribute

Is there a known way to implement this, either within bilge, or using an external crate to assist?

The bitfield-struct crate provides a way to set a field default, and because it uses a builder-lite style of initialisation it's easy enough to omit overwriting the defaulted field. I'm not sure how the bilge API for struct MyStruct::new(...) would work with a default field since it would have to be provided.

So maybe we could consider this a feature request for a "fixed" field, rather than a default? In my example above, the opcode field would always be the value "8", so it wouldn't need to be set by a new() call. Maybe something like:

    #[bitsize(128)]
    #[derive(DebugBits, FromBits)]
    struct AddInstruction {
        src1: u4,
        reserved: u22,
        #[fixed(Value = ADD_OPCODE)]
        opcode: u6,
        src2: u4,
        reserved: u28,
        dst: u4,
        reserved: u60,
    }

Then the fixed field would be omitted from the new(...) parameter list, in the same way that reserved is:

    let i = AddInstruction::new(
        u4::new(src1),
        u4::new(src2),
        u4::new(dst),
    );

But the field opcode could still be accessed, as a read-only value.

hecatia-elegua commented 3 months ago

86 seems relevant.

External derive macros can't work on fields, since they are compressed before being passed on, only custom derives with a name ending in Bits are applied while the fields still exist.

If I had the time, I would try to implement BuilderBits, which would provide a type-safe builder pattern, then add this sort of thing to it. The base macro should stay small and mostly syntax compatible to normal rust code, which does not have default fields.

Oh and is the only usecase here that you don't have to fill in new (or the builder)?

DavidAntliff commented 3 months ago

Yes, #86 does seem relevant. Thank you for your reply.

The main use-case for me is that I'm building a large set of these structs, to represent a hardware interface (FPGA registers and CPU instructions), and in some cases there are bit patterns that are fixed and must be present. Some of these are non-zero defaults that can be modified (like a control bit that is typically set), and some are genuinely fixed values (like an instruction's OpCode). What typically happens is user code creates an instance of the register/instruction representation and then converts it into an unsigned integer for volatile writing to device memory.

While I can definitely keep these structs private and put them behind a functional interface, I had hoped to make them directly instantiable and usable by the users of my library. Without the ability to set fixed values (or defaults), the user becomes responsible to set this themselves, and that opens up a class of bugs where the wrong values are accidentally used, or not even set at all.

In contrast, bitfield-struct has defaults that can be omitted when building the struct. Sure, someone can accidentally overwrite, say, the not-actually-fixed "opcode" field with a nonsense value, but they have to opt-in to this mistake, rather than simply forget to do something. Default field values are automatically applied.

Your builder-pattern idea would be similar and I think would be a good API. I think you'd want to have some fields that must be provided for the builder to succeed, and others that are optional and adopt default values if not specified. I think it would be useful to also have truly fixed fields that are set in the struct definition and can't be changed by the builder API - that would be one step better than bitfield-struct.

Would you be open to a PR for BuilderBits? Do you have any further thoughts/plans for this? I could consider contributing if you're open to it. I have very little experience with writing macros, but happy to learn.

hecatia-elegua commented 3 months ago

This probably means we need a way to deactivate generation of new too.

If you only had ::default() as a method to construct e.g. AddInstruction, that would work too. The problem with that is when you impl Default for AddInstruction, you have to work on raw bits. I think adding #[default(some expression)] would be the easiest way to handle that part.

truly fixed fields that are set in the struct definition and can't be changed by the builder API

The best way to handle that is with another feature we don't have yet: read-only and write-only, see #9, but if it should not be readable either, I'm not sure if the feature should actually be no-read and no-write instead... or ro, wo and "fixed"?

Sketching something up:

#[bitsize(128)]
#[derive(DebugBits, FromBits, DefaultBits)]
struct AddInstruction {
    src1: u4,
    reserved: u22,
    // this is split into two attributes: using fixed alone could mean "the hardware sets this to some value, we don't know the default"
    #[fixed] // or #[read_only]
    #[default(ADD_OPCODE)]
    opcode: u6,
    src2: u4,
    reserved: u28,
    dst: u4,
    reserved: u60,
}

let mut add = AddInstruction::default();
add.set_src1(a);
add.set_src2(b);
add.set_dst(c);
// or
let add = AddInstruction::builder().default().src1(a).src2(b).dst(c).build();
// or probably more safe to call default internally in builder? or (somehow) forcing a call to default if not all fields are set
let add = AddInstruction::builder().src1(a).src2(b).dst(c).build();
// or, if #[fixed] is used anyways
let add = AddInstruction::new(a, b, c);

The builder pattern integrates with default, it's mostly about having named arguments, then verifying that all fields have been set. I did not like modular_bitfield's builder pattern because it left verification away, but this one is great: https://github.com/danlehmann/bitfield/pull/32.

some fields that must be provided for the builder to succeed others that are optional and adopt default values if not specified

every field is provided, but default handles others

hecatia-elegua commented 3 months ago

And contribution would be very cool! Tell me what you think of the general idea here, I believe #[default(..)] in DefaultBits and #[read_only] in the base macro should be enough for your usecase. The good builder implementation generates const generics from macro land, that might be a bit hardcore.

Some pointers for how the internals work: https://github.com/hecatia-elegua/bilge/blob/main/bilge-impl/src/bitsize/split.rs#L7-L42 (we should probably document this more openly somewhere) https://github.com/hecatia-elegua/bilge/blob/main/bilge-impl/src/default_bits.rs (relevant to the usecase)

DavidAntliff commented 3 months ago

Hi @hecatia-elegua - due to business pressure I have had to move on and use bitfield-struct, in order to meet my requirements and deadline. This means I haven't been able to deploy bilge in my project, so my familiarity with it hasn't grown as I had hoped. Thus, I'm not really in a good position to contribute at the moment, but I will likely revisit bilge in future.

FWIW, this "default" issue was ultimately less of an issue than the nested structs (#91) which I used heavily in bitfield-struct as many of my hardware registers share many common fields.

hecatia-elegua commented 2 months ago

@DavidAntliff that's alright. If you got the motivation, tell me if https://github.com/hecatia-elegua/bilge/issues/91#issuecomment-2134891067 would work for you.