Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose demultiplexed boolean properties for DIO registers #25

Open
1 of 4 tasks
glopesdev opened this issue Dec 20, 2023 · 2 comments
Open
1 of 4 tasks

Expose demultiplexed boolean properties for DIO registers #25

glopesdev opened this issue Dec 20, 2023 · 2 comments
Labels
proposal Request for a new feature

Comments

@glopesdev
Copy link
Collaborator

glopesdev commented Dec 20, 2023

  • Proposed
  • Prototype: Not Started
  • Implementation: Not Started
  • Specification: Not Started

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:

  • Several published devices exist which do not follow this proposal, which would mean either creating conflicting and possibly confusing ways of using different devices, or breaking the published interfaces of existing devices to conform to this proposal.
  • At the level of firmware generation, it is easy to see how simple bitmask types might be leveraged to automatically generate C/C++ enum types. It is not clear whether/how we want to support automatic generation of payloadSpec types in C/C++ firmware code.

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

  • TBD
@glopesdev glopesdev added the proposal Request for a new feature label Dec 20, 2023
@glopesdev glopesdev changed the title Consider exposing demultiplexed boolean payloads instead of bitmasks for digital IO registers Consider exposing demultiplexed boolean payloads instead of bitmasks for DIO registers Dec 20, 2023
@glopesdev glopesdev changed the title Consider exposing demultiplexed boolean payloads instead of bitmasks for DIO registers Expose demultiplexed boolean properties instead of bitmasks for DIO registers Dec 20, 2023
@glopesdev glopesdev changed the title Expose demultiplexed boolean properties instead of bitmasks for DIO registers Expose demultiplexed boolean properties for DIO registers Dec 20, 2023
@bruno-f-cruz
Copy link
Member

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
Copy link
Member

bruno-f-cruz commented Jan 18, 2024

Consider:

  • This is a subset of the struct-like payloads:
  • Allow users to define multiple operators per register (might allow back-compatibility)
  • waiting for a struct demo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Request for a new feature
Projects
None yet
Development

No branches or pull requests

2 participants