[Johannes-Thiel]

Motivation

A host reads module properties less often than a module's state machine changes state, so it misses intermediate states: a short move that goes from idle through accepted and moving back to idle between two reads looks like nothing happened.
This PR adds a reusable state counter whose value only climbs, so a host can tell from any later reading whether a state was reached, even if it never saw it; the RMD motor is meant to be its first user in a separate PR.

Implementation

The ladder

• StateCounter<State> is an IntegerVariable, templated on the module's state enum, which ends with a COUNT sentinel one past the last state.
• The value is a ladder: the low bits hold the current state (as many as the number of states needs, e.g. 2 bits for 3 states), the bits above hold the epoch, the number of completed cycles. So every state of every cycle has its own rung, and the rungs are ordered by time.
• set(state) is the only way to change it: a later state keeps the epoch, an earlier state starts the next epoch, and staying in the same state changes nothing. The value therefore never goes back, and skipped states are simply passed.
• A host that waits for a state computes its rung and compares the current reading against it: once the reading is at or above that rung, the state was reached or passed, no matter how many states happened between two reads.
• The value is 8 bits and wraps at 256; is_after(a, b) compares two values across the wrap, which is valid as long as they are less than 128 steps apart. state() and epoch() read the two parts.
• How a module reports its commands on the counter (e.g. which state is idle, and whether an accepted command first returns to idle) is left to the module and decided in the PR of the first module that uses it.
• A static_assert requires at least 2 states and a full cycle to fit in half a period (at most 64 states); set() throws on a state out of range.

Changed files

• main/utils/state_counter.h: new header-only StateCounter<State>.
• docs/module_reference.md: new section "State Counters" with the bit layout, the set() rule and how a host compares values.

Open: the property is still writable from the host through set_integer_value, which bypasses set() and can move the value backwards. Making set_integer_value virtual and routing it to set() would close that, but changes Variable for all modules. A safer alternative is to forbid the setting of set_integer_value for this counter.

Progress

• The implementation is complete.
• Documentation has been updated (or is not necessary).
• Tested on hardware (or is not necessary).

No module uses the counter yet, so there is nothing to test on hardware, and since no source file includes the header, the firmware build does not compile it either.
A host test (clang++, C++17, minimal stand-in for IntegerVariable) checked the counting rule for a 3-state enum (later state, earlier state, staying, wrap past 256), is_after() across the wrap, the out-of-range throw, and the static_assert (64 states compile, 65 do not); the header also compiles against the real variable.h.

※

[ftilde]

As discussed with Johannes, I "kneaded" the design some more. 6d85a88 i'm kind of on the fence about, but leaning towards keeping it since it simplifies the host side.

Another open point: If we make set_integer_value virtual, we can point it to set so that host-side writes also obey the increasing rule, but I'd want to clear that with Falko.

Also, here is some example host code for motivation:

from __future__ import annotations

from dataclasses import dataclass
from enum import IntEnum
from typing import Generic, TypeVar

VALUE_BITS = 8
PERIOD = 1 << VALUE_BITS

S = TypeVar('S', bound=IntEnum)


def is_after(a: int, b: int) -> bool:
    """Whether counter value `a` came after `b`; only defined for values less than half a period apart."""
    distance = (a - b) % PERIOD
    return 0 < distance < PERIOD // 2


@dataclass(frozen=True)
class Counter(Generic[S]):
    """A reading of a Lizard state counter for an IntEnum with a COUNT sentinel."""
    states: type[S]
    value: int

    @property
    def state_bits(self) -> int:
        return (self.states.COUNT - 1).bit_length()

    @property
    def state(self) -> S:
        return self.states(self.value & ((1 << self.state_bits) - 1))

    @property
    def epoch(self) -> int:
        return self.value >> self.state_bits

    def next(self, state: S) -> Counter[S]:
        """The reading after the module sets `state`: an earlier state starts the next epoch."""
        if state == self.state:
            return self
        epoch = self.epoch + (1 if state < self.state else 0)
        return Counter(self.states, ((epoch << self.state_bits) | state) % PERIOD)

    def __gt__(self, other: Counter[S]) -> bool:
        return is_after(self.value, other.value)

    def __ge__(self, other: Counter[S]) -> bool:
        return self == other or is_after(self.value, other.value)

    def __lt__(self, other: Counter[S]) -> bool:
        return is_after(other.value, self.value)

    def __le__(self, other: Counter[S]) -> bool:
        return self == other or is_after(other.value, self.value)


if __name__ == '__main__':
    class Motor(IntEnum):
        IDLE = 0
        MOVING = 1
        COUNT = 2

    readings = iter([0, 1, 1, 2, 2])  # what the module reports on each poll; drop the 1s to miss "moving"

    def read() -> Counter[Motor]:
        return Counter(Motor, next(readings))

    initial = read()
    assert initial.state == Motor.IDLE
    # send the move command
    moving = initial.next(Motor.MOVING)
    while read() < moving:
        pass
    print('moving')
    moved = moving.next(Motor.IDLE)
    while read() < moved:
        pass
    print('moved')

And example module code:

#include "state_counter.h"
#include <cstdio>

enum class MotorState { IDLE, MOVING, COUNT };

struct Motor {
    StateCounter<MotorState> state; // the module's "state" property
    int remaining = 0;

    void move(const int duration) {
        if (this->state.state() != MotorState::IDLE) {
            throw std::runtime_error("motor is already moving");
        }
        this->state.set(MotorState::MOVING);
        this->remaining = duration;
    }

    void step() {
        if (this->remaining > 0 && --this->remaining == 0) {
            this->state.set(MotorState::IDLE);
        }
    }
};

[falkoschindler]

Just a quick look in between, not a review yet.

My main question is what exactly this is for. I understand the general idea from the motivation and from the first commit message: a host misses short moves between two reads and can't tell them apart from refused ones. But I can't find the concrete case behind it. RmdMotor has no state machine today, not even an idle property, and there is no PR or issue yet that shows which machine, which commands and which host loop this has to serve. Without that, I find it hard to judge the design. The interesting decisions (command convention, write protection, value width) have already been moved to "the PR of the first module that uses it", and nothing includes the header, so the firmware build doesn't even compile it. Could we put the RMD part into this PR, or open it and merge both together? Then the counter can be reviewed against a real user.

Two things to keep in mind for that:

• Host writes don't go through set_integer_value. m.state = 5 from the console or a rule ends up in Module::write_property → Variable::assign, which writes the slot directly, so making set_integer_value virtual wouldn't close the gap. BusTelemetry::write_property (main/modules/bus_telemetry.cpp (56)) already shows a way that doesn't touch Variable: it rejects its mirrored properties as read-only. The module using the counter could do the same.
• The ladder assumes the machine runs through its states in enum order. If a module jumps from idle to the third state, the second one counts as "passed", which is why the stepper motor was dropped again. Whether the RMD machine fits that, and whether 8 bits with the wrap and is_after on every host are really needed, is easier to settle with the module in front of us.

@Johannes-Thiel @ftilde

※

[ftilde]

Concrete case is state in here: https://github.com/zauberzeug/lizard-zz/blob/main/components/lizard_private/rmd_delta_arm.cpp

The ladder assumes the machine runs through its states in enum order

Not really. In the mentioned example (1 -> 3 -> 2) the move to 2 increases the epoch, but that's fine. The only purpose of the ladder is that we don't miss a transition x -> y -> ... -> x. The ladder is just a simple way to ensure that.

One thing that this made me think about, though (and maybe this is what you were talking about?): The host side

    moved = moving.next(Motor.IDLE)
    while read() < moved:
        pass

is not robust to changes to the lizard side (1 -> 3 -> 2 example). What we want instead (maybe with some sugar):

    while True:
        r = read()
        if r.epoch().is_after(inital.epoch()) && r.state == State.IDLE:
            break

Host writes don't go through set_integer_value

Hm, good point, I just assumed that... So ensuring that is up to the user (i.e. each module) I guess? We could make it safer by adding a read_only property to Variable (fits into the niche after type I think) and raise in assign, but I'm not sure if that's really a good idea. Ideally I'd like a compile error for assign on StateCounter, but that would only work if it's not a Variable I guess.

[falkoschindler]

Thanks, that makes it a lot clearer. Since the only user is RmdDeltaArm in lizard-zz, I'd rather keep the counter there for now, next to the delta arm. Before we generalize something like this and move it into the core library, I'd like to see it needed at least twice. Then we know what exactly is common and what each module needs, and we don't have to guess the general shape from a single case. If a public module ever needs the same, the counter can come back to Lizard together with that user.

Moving it there also settles the other points without touching Lizard: the delta arm can reject writes to state in its own write_property, like BusTelemetry does, so no read_only flag in Variable is needed. The host recipe ("next epoch and state is idle" rather than comparing against a precomputed rung) and the command convention (a new move while still "received" would be 1 → 1 and count nothing) can then be documented and decided right where they apply.

So I'd suggest closing this PR and opening it against lizard-zz instead. @Johannes-Thiel @ftilde, does that work for you?

※