harp-tech / protocol

Description of the Harp protocol.
https://harp-tech.org/protocol/BinaryProtocol-8bit.html
MIT License
3 stars 5 forks source link

Expose demultiplexed boolean properties for DIO registers #25

Open glopesdev opened 6 months ago

glopesdev commented 6 months ago

Summary

Use payloadSpec to represent DIO inputs. This would allow much easier selection of boolean flags directly as fields using a member selector, and also improve property grid experience when creating command message payloads.

Motivation

The current standard for representing DIO registers in the device.yml file is to define a custom bitmask enum type over the register number. For example:

  DigitalInputState:
    address: 32
    access: Event
    type: U8
    maskType: DigitalInputs
    description: Reflects the state of DI digital lines of each Port
...
  DigitalInputs:
    description: Specifies the state of port digital input lines.
    bits:
      DIPort0: 0x1
      DIPort1: 0x2
      DIPort2: 0x4
      DI3: 0x8

The above will be represented at the level of the high-level interface as a C# enum type with the [Flags] attribute. Values from flags can be hard to manipulate, requiring specialized operators to decode or encode the values (HasFlag). Even when using such operators, the chain of processing can be complex for something as simple as retrieving the timestamped events of when a single DI port changes value:

workflow

The ConvertTimestamped node is required in this case to make sure the timestamp is carried through the conversion operator with HasFlag which will be inside the group node to extract the single bit from the DIO bitmask state.

Detailed Design

In this proposal, representation of the same DIO register in the device.yml file would be:

  DigitalInputState:
    address: 32
    access: Event
    type: U8
    payloadSpec:
      DIPort0:
        mask: 0x1
        interfaceType: bool
      DIPort1:
        mask: 0x2
        interfaceType: bool
      DIPort2:
        mask: 0x4
        interfaceType: bool
      DI3:
        mask: 0x8
        interfaceType: bool
    description: Reflects the state of DI digital lines of each Port

With this specification for the register, the generator would create a DigitalInputStatePayload type with the following layout:

public struct DigitalInputStatePayload
{
    public bool DIPort0;
    public bool DIPort1;
    public bool DIPort2;
    public bool DI3;
}

This then becomes the output type of the Parse node, which means that individual bit flags can be selected with the right-click context menu. It also means that the CreateMessage operator for this register will expose these properties directly as fields in the grid, where it becomes easy to see and change which bits are being set or cleared.

The proposal requires no change in the current implementation of the standard, in so much as it is only a change in the way the existing standard is used to model specific types of registers.

Drawbacks

There are a few important reasons why we might not want to do this:

Alternatives

Manipulating DIO registers is a fundamental part of many boards, and the existing approaches for manipulating bitmasks seem too confusing for such a basic recurrent feature. The impact of not doing anything to improve the situation will cause significant friction in the learning curve of Harp high-level interfaces.

There might be another alternative to work around the problem without breaking current device compatibility and keeping the default register types as they are. This should be explored in detail elsewhere, but briefly we could mark specific bitmasks as "demux types" and have the generator automatically create conversion operators that convert to/from bitmask type to a demux payload type.

New virtual register types could be added to the drop-down of Parse so that the return type is a demuxed structure. For the CreateMessage operator we could also create a new type converter that exposes bit flags as nested properties so it becomes easier to toggle individual bits HIGH or LOW.

Marking of bitmasks as demux types could be done either at the level of the bitmask type itself, or at the level of the register, for example by specifying simultaneously the maskType and the payloadSpec attributes.

Unresolved Questions

Whether this will replace the existing specification, or be included as an addition to the existing specification remains to be seen, depending on the discussion of the alternatives presented above.

Design Meetings

bruno-f-cruz commented 6 months ago

If it is worth it, I don't mind implementing this change as a breaking change. I think it's currently a very core and annoying pattern that takes too many operators to get right.

Regarding the yml spec, I would suggest that rather than re-defining the schema, we could have an additional property in each register: bitmask_compiler that toggles between these two modes: "FlagEnum" (default) or "PayloadSpec" (new proposal). As you pointed out, some registers still make more sense to be Flag Enums. In both situations they should be coded as bitmasks in the yml file since they share the same firmware implementation.

We should keep in mind that the yml file should also serve as a blueprint that people can use to generate firmware and, worst case scenario, as a lookup table to interact with the device via the generic bonsai interface when needed.

bruno-f-cruz commented 5 months ago

Consider: