Clean up test helpers

.github/workflows/no-fixmes.yml

@@ -0,0 +1,25 @@
+name: no fixmes
+
+on:
+  pull_request:
+  push:
+    branches: [ main, devel ]
+# Cancel in-progress runs if a newer run is started on a given PR
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: ${{ !contains(github.ref, 'devel') && !contains(github.ref, 'release/')}}
+
+jobs:
+  no-fixmes:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Fail on FIXME markers in src or test
+        # The marker string fails the check whatever the language. git grep
+        # searches only tracked files and does not descend into submodules.
+        # Exempt a file by adding an ":(exclude)path" pathspec to the command.
+        run: |
+          if git grep -n "FIXME" -- src test; then
+            echo "::error::FIXME markers found (see log); resolve them before merging"
+            exit 1
+          fi

README.md

@@ -240,7 +240,7 @@ prm_3: U8 = Ref.sendBuffComp.parameter3
 
 A significant limitation of this is that it will only return the value most recently saved to the parameter database. This means you must command `_PRM_SAVE` before the sequence will see the new value.
 
-> Note:  If a telemetry channel and parameter have the same fully-qualified name, the fully-qualified name will get the value of the telemetry channel
+> Note: If two dictionary items have the same fully-qualified name, the name resolves to the first match in this order: telemetry channel, parameter, enum constant, FPP constant.
 
 ## Conditionals
 Fpy supports comparison operators:
@@ -759,7 +759,6 @@ The wasm harness is built automatically at the start of the test session, with a
 
 Tests marked with `@pytest.mark.wasm` are end-to-end LLVM/wasm tests and always run on the wasm backend (with the same requirements as above), even when `--wasm` is not passed.
 
-# FIXME I'd like to remove the use-gds feature
 ### `--use-gds`
 
 By default, tests run against a local `Svc::FpySequencer` through the harness. Passing `--use-gds` runs sequences against a live F Prime GDS deployment instead; see [Running on a test F Prime deployment](#running-on-a-test-f-prime-deployment) for how to set one up and the full command line (a `--dictionary` argument is also required).

src/fpy/bytecode/assembler.py

@@ -17,16 +17,17 @@
 fpybc_grammar_str = (Path(__file__).parent / "grammar.lark").read_text(encoding="utf-8")
 
 
-def parse(text: str):
-    parser = Lark(
-        fpybc_grammar_str,
-        start="input",
-        parser="lalr",
-        propagate_positions=True,
-        maybe_placeholders=True,
-    )
+_fpybc_parser = Lark(
+    fpybc_grammar_str,
+    start="input",
+    parser="lalr",
+    propagate_positions=True,
+    maybe_placeholders=True,
+)
 
-    tree = parser.parse(text)
+
+def parse(text: str):
+    tree = _fpybc_parser.parse(text)
     transformed = FpyBcTransformer().transform(tree)
     return transformed
 

src/fpy/codegen_fpybc.py

@@ -148,10 +148,10 @@ class FpybcBackendState(BackendState):
     """The fpybc backend's view of a program: how its variables are laid out,
     and what it has emitted so far."""
 
-    frame_offsets: dict[VariableSymbol, int] = field(default_factory=dict)
+    variable_frame_offsets: dict[VariableSymbol, int] = field(default_factory=dict)
     """variable to the offset of its storage within its frame"""
 
-    frame_sizes: dict[Ast, int] = field(default_factory=dict)
+    block_frame_sizes: dict[AstBlock, int] = field(default_factory=dict)
     """the block that owns a frame, to the total size in bytes of that frame's
     locals"""
 
@@ -187,7 +187,7 @@ def __init__(self, offset: int):
         self.offset = offset
 
     def visit_AstBlock(self, node: AstBlock, state: CompileState):
-        frame_offsets = state.backend.frame_offsets
+        frame_offsets = state.backend.variable_frame_offsets
         for sym in state.enclosing_scope[node].group(NameGroup.VALUE).values():
             if is_instance_compat(sym, VariableSymbol) and sym not in frame_offsets:
                 frame_offsets[sym] = self.offset
@@ -223,12 +223,25 @@ def run(self, start: Ast, state: CompileState):
         super().run(start, state)
 
     def visit_AstDef(self, node: AstDef, state: CompileState):
-        self._layout_function_frame(node, state)
+        # Formal parameters sit before the frame start, at negative offsets.
+        frame_offsets = state.backend.variable_frame_offsets
+        func_sym = state.resolved_symbols[node.name]
+        body_values = state.enclosing_scope[node.body].group(NameGroup.VALUE)
+        arg_offset = -STACK_FRAME_HEADER_SIZE
+        for arg_name, arg_type, _default in reversed(func_sym.args):
+            arg_offset -= arg_type.max_size
+            arg_var = body_values[arg_name]
+            assert is_instance_compat(arg_var, VariableSymbol), arg_var
+            frame_offsets[arg_var] = arg_offset
+
+        state.backend.block_frame_sizes[node.body] = self._layout_locals(
+            node.body, 0, state
+        )
 
     def _layout_main_frame(self, state: CompileState):
         # Sequence args arrive on the stack first, then the flags slot -- which
         # lives in the base scope but occupies a slot in the main frame here.
-        frame_offsets = state.backend.frame_offsets
+        frame_offsets = state.backend.variable_frame_offsets
         offset = 0
         for name, arg_type in state.this_seq_arg_specs:
             arg_var = state.main_scope.group(NameGroup.VALUE)[name]
@@ -237,23 +250,10 @@ def _layout_main_frame(self, state: CompileState):
         frame_offsets[state.flags_var] = offset
         offset += state.flags_var.type.max_size
 
-        state.backend.frame_sizes[state.main_block] = self._layout_locals(
+        state.backend.block_frame_sizes[state.main_block] = self._layout_locals(
             state.main_block, offset, state
         )
 
-    def _layout_function_frame(self, node: AstDef, state: CompileState):
-        # FIXME you can inline this func
-        # Formal parameters sit before the frame start, at negative offsets.
-        frame_offsets = state.backend.frame_offsets
-        func = state.resolved_symbols[node.name]
-        body_values = state.enclosing_scope[node.body].group(NameGroup.VALUE)
-        arg_offset = -STACK_FRAME_HEADER_SIZE
-        for arg_name, arg_type, _default in reversed(func.args):
-            arg_offset -= arg_type.max_size
-            frame_offsets[body_values[arg_name]] = arg_offset
-
-        state.backend.frame_sizes[node.body] = self._layout_locals(node.body, 0, state)
-
     def _layout_locals(self, frame_block: AstBlock, offset: int, state) -> int:
         """Lay out every local in *frame_block*'s frame, starting at *offset*,
         and return the offset past the last one (the frame's total size)."""
@@ -280,7 +280,7 @@ def visit_AstDef(self, node: AstDef, state: CompileState):
         code = [entry_label]
 
         # Allocate space for local variables
-        frame_size_bytes = state.backend.frame_sizes[node.body]
+        frame_size_bytes = state.backend.block_frame_sizes[node.body]
         if frame_size_bytes > 0:
             code.append(AllocateDirective(frame_size_bytes))
 
@@ -506,7 +506,7 @@ def _emit_assert_cmd_response_ok(
         dirs.append(not_ok_label)
         # response was not OK — read flags.assert_cmd_success from the stack
         # assert_cmd_success is at offset 0 within the flags struct
-        flag_offset = state.backend.frame_offsets[state.flags_var]
+        flag_offset = state.backend.variable_frame_offsets[state.flags_var]
         dirs.append(LoadAbsDirective(flag_offset, BOOL.max_size))
         # if flag is false, skip to end (don't exit)
         dirs.append(IrIf(end_label))
@@ -927,7 +927,7 @@ def emit_AstIdent(self, node: AstIdent, state: CompileState):
         # a global variable. At top level, stack_frame_start = 0, so a
         # frame-relative offset is already the absolute one.
         use_abs = self.in_function and sym.is_global
-        offset = state.backend.frame_offsets[sym]
+        offset = state.backend.variable_frame_offsets[sym]
         if use_abs:
             dirs = [LoadAbsDirective(offset, sym.type.max_size)]
         else:
@@ -1280,12 +1280,12 @@ def emit_AstAssign(self, node: AstAssign, state: CompileState):
         dynamic_components = []
 
         if is_instance_compat(lhs, VariableSymbol):
-            base_frame_offset = state.backend.frame_offsets[lhs]
+            base_frame_offset = state.backend.variable_frame_offsets[lhs]
             is_global_var = lhs.is_global
         else:
             assert is_instance_compat(lhs, FieldAccess), lhs
             assert is_instance_compat(lhs.base_sym, VariableSymbol), lhs.base_sym
-            base_frame_offset = state.backend.frame_offsets[lhs.base_sym]
+            base_frame_offset = state.backend.variable_frame_offsets[lhs.base_sym]
             is_global_var = lhs.base_sym.is_global
 
             # Walk the field access chain to compute the total offset.
@@ -1413,14 +1413,16 @@ def emit_AstBlock(self, node: AstBlock, state: CompileState):
 
         flags_type = state.flags_var.type
         args_size = sum(t.max_size for _, t in state.this_seq_arg_specs)
-        assert state.backend.frame_offsets[state.flags_var] == args_size
+        assert state.backend.variable_frame_offsets[state.flags_var] == args_size
         flags_default = FpyValue(flags_type, dict(flags_type.member_defaults))
         main_body.append(PushValDirective(flags_default.serialize()))
 
         # we can calc how much space the user-defined lvars take by subtracting
         # the sequence args size, and the flags size, from the frame size
 
-        remaining = state.backend.frame_sizes[node] - flags_type.max_size - args_size
+        remaining = (
+            state.backend.block_frame_sizes[node] - flags_type.max_size - args_size
+        )
         assert remaining >= 0, remaining
 
         # allocate space for local variables

src/fpy/harness.py

@@ -12,7 +12,7 @@
 from pathlib import Path
 
 REPO_ROOT = Path(__file__).parent.parent.parent
-FPY_HARNESS_BINARY = (
+FPYBC_HARNESS_BINARY = (
     REPO_ROOT / "build-artifacts" / "Linux" / "FpyHarness" / "bin" / "FpyHarness"
 )
 _WASM_HARNESS_ROOT = REPO_ROOT / "test" / "harness" / "wasm"
@@ -31,8 +31,7 @@
     a malformed reply, or crashed. Distinct from a sequence failing."""
 
 
-# FIXME this should be called build fpybc harness
-def build_harness() -> None:
+def build_fpybc_harness() -> None:
     """Builds the FpySequencer harness executable from the fprime submodule.
     The build is incremental, so this is cheap when nothing changed."""
     if not (REPO_ROOT / "test" / "fprime" / "CMakeLists.txt").exists():
@@ -135,7 +134,7 @@
     def _start(self) -> None:
         if not self._binary.exists():
             raise HarnessError(
-                f"harness binary {self._binary} does not exist; build it with build_harness()"
+                f"harness binary {self._binary} does not exist; build it first"
             )
         self._stderr_file = tempfile.TemporaryFile(mode="w+")
         self._process = subprocess.Popen(
@@ -161,44 +160,52 @@
         self._stderr_file = None
 
 
-_fpy_harness: SequencerHarness | None = None
+_fpybc_harness: SequencerHarness | None = None
 _wasm_harness: SequencerHarness | None = None
-# The first failed build, re-raised on later calls: retrying the build once
+# The first failed builds, re-raised on later calls: retrying a build once
 # it has failed only repeats the same slow failure.
-_fpy_build_error: HarnessError | None = None
+_fpybc_build_error: HarnessError | None = None
+_wasm_build_error: HarnessError | None = None
 
 
-# FIXME: should be fpybc
-def fpy_harness() -> SequencerHarness:
+def fpybc_harness() -> SequencerHarness:
     """The shared harness for the fpy bytecode backend, building its binary
     on first use."""
-    global _fpy_harness, _fpy_build_error
-    if _fpy_build_error is not None:
-        raise _fpy_build_error
-    if _fpy_harness is None:
+    global _fpybc_harness, _fpybc_build_error
+    if _fpybc_build_error is not None:
+        raise _fpybc_build_error
+    if _fpybc_harness is None:
         try:
-            build_harness()
+            build_fpybc_harness()
         except HarnessError as e:
-            _fpy_build_error = e
+            _fpybc_build_error = e
             raise
-        _fpy_harness = SequencerHarness(FPY_HARNESS_BINARY)
-    return _fpy_harness
+        _fpybc_harness = SequencerHarness(FPYBC_HARNESS_BINARY)
+    return _fpybc_harness
 
 
 def wasm_harness() -> SequencerHarness:
-    """The shared harness for the LLVM/wasm backend."""
-    global _wasm_harness
+    """The shared harness for the LLVM/wasm backend, building its binary on
+    first use."""
+    global _wasm_harness, _wasm_build_error
+    if _wasm_build_error is not None:
+        raise _wasm_build_error
     if _wasm_harness is None:
+        try:
+            build_wasm_harness()
+        except HarnessError as e:
+            _wasm_build_error = e
+            raise
         _wasm_harness = SequencerHarness(WASM_HARNESS_BINARY)
     return _wasm_harness
 
 
 def close_all() -> None:
     """Stops any running harness processes (end of the test session)."""
-    global _fpy_harness, _wasm_harness
-    if _fpy_harness is not None:
-        _fpy_harness.close()
-        _fpy_harness = None
+    global _fpybc_harness, _wasm_harness
+    if _fpybc_harness is not None:
+        _fpybc_harness.close()
+        _fpybc_harness = None
     if _wasm_harness is not None:
         _wasm_harness.close()
         _wasm_harness = None

src/fpy/state.py

@@ -507,6 +507,19 @@ def _populate_type_defaults(typ: FpyType) -> None:
         typ.elem_defaults = tuple(array_defaults)
 
 
+def is_seq_run_cmd(cmd) -> bool:
+    """Whether a command is a sequence-run command, detected by its signature:
+    (fileName: string, block: Svc.BlockState, args: Svc.SeqArgs). The user
+    provides the target sequence's arguments in place of the SeqArgs param."""
+    args = cmd.arguments
+    return (
+        len(args) == 3
+        and args[0][2].is_string
+        and args[1][2].name == "Svc.BlockState"
+        and args[2][2].name == "Svc.SeqArgs"
+    )
+
+
 def _make_type_ctor(name: str, typ: FpyType) -> TypeCtorSymbol | None:
     """Create a TypeCtorSymbol for a type, or return None if it has no callable ctor."""
     if typ.kind == TypeKind.STRUCT:
@@ -548,6 +561,7 @@ def _build_global_scopes(dictionary: str) -> tuple:
     _validate_and_replace_type(
         dict_type_name_dict, "Fw.TimeComparison", TIME_COMPARISON
     )
+    _validate_and_replace_type(dict_type_name_dict, "Fw.LogSeverity", LOG_SEVERITY)
     _validate_and_replace_type(dict_type_name_dict, "Svc.BlockState", BLOCK_STATE)
     _update_seq_args_from_dict(dict_type_name_dict)
 
@@ -563,7 +577,6 @@ def _build_global_scopes(dictionary: str) -> tuple:
         BOOL.name: BOOL,
         CHECK_STATE.name: CHECK_STATE,
         FLAGS_TYPE.name: FLAGS_TYPE,
-        LOG_SEVERITY.name: LOG_SEVERITY,
     }
 
     # Collect enum constants from the final type dict (after builtins and
@@ -585,14 +598,7 @@ def _build_global_scopes(dictionary: str) -> tuple:
 
     for name, cmd in cmd_name_dict.items():
         args = [(arg_name, arg_type, None) for arg_name, _, arg_type in cmd.arguments]
-        # Detect sequence-run commands by matching the 3-arg signature:
-        # (fileName: string, block: Svc.BlockState, args: Svc.SeqArgs)
-        if (
-            len(args) == 3
-            and args[0][1].is_string
-            and args[1][1].name == "Svc.BlockState"
-            and args[2][1].name == "Svc.SeqArgs"
-        ):
+        if is_seq_run_cmd(cmd):
             # Strip the SeqArgs param; user provides varargs instead
             fixed_args = args[:2]
             callable_name_dict[name] = CommandSymbol(