fix(ui): make gate capture ask for the gates a circuit actually has, and say why a save was refused

CHANGELOG.md

@@ -364,6 +364,25 @@ The project follows Semantic Versioning once the first firmware release is tagge
 
 ### Fixed
 
+- gate capture **stopped asking for eight endpoints that no longer exist.** Removing pit
+  entry and exit (#227) took the circuit from four gates to two, and the controller
+  followed - but the text it hands the screen did not, so the device went on instructing
+  the driver to capture 8 endpoints across FOUR GATES while save unlocked at four across
+  two, and the selection line counted up to `8` it could never reach. The count is now
+  taken from `kGateCaptureEndpointCount`, or the gates are named instead of counted
+  (`CAPTURE BOTH ENDS OF THE START AND FINISH LINES`), and a host test asserts the
+  instructions against the stated shape rather than a number typed into it (#204)
+- a capture refused by validation now **says which check refused it.** `SAVE BLOCKED - GATE
+  GEOMETRY FAILED SAFETY VALIDATION` named none of the three things that could have gone
+  wrong, to somebody standing at a circuit with the engine off. A line whose ends are under
+  a metre apart, a line outside the circuit's geofence and a line too long or too close to
+  the direction of travel have nothing in common but the refusal, and three different
+  remedies. `serialize_track_definition` already round-trips its candidate through
+  `load_track_definition` - that is what keeps invalid geometry off the card - so the
+  report it was discarding is now passed back and both the panel and the log name it. Where
+  one result covers several checks, all of them are listed rather than one being picked and
+  presented as the cause (#204)
+
 - the rule sending a running session to **Race's screen rather than Track Day's is now a
   host-tested function** instead of a comparison inside the router's screen lookup. Track
   Day withholding live lap times is a compliance rule, and the screen it is drawn on has

firmware/components/track/definition.cpp

@@ -1262,7 +1262,8 @@ bool write_gate(JsonWriter& writer, const DirectedGateDefinition& gate) noexcept
 }  // namespace
 
 TrackSerializeResult serialize_track_definition(
-    const TrackDefinition& definition, TrackDefinitionBlob& output) noexcept
+    const TrackDefinition& definition, TrackDefinitionBlob& output,
+    TrackLoadReport* const refusal) noexcept
 {
     TrackDefinitionBlob candidate{};
     JsonWriter writer{candidate};
@@ -1305,9 +1306,12 @@ TrackSerializeResult serialize_track_definition(
         return TrackSerializeResult::capacity_exceeded;
     }
     TrackDefinition round_trip{};
-    if (load_track_definition(
-            {candidate.bytes.data(), candidate.size}, round_trip).result !=
-        TrackLoadResult::loaded) {
+    const auto report =
+        load_track_definition({candidate.bytes.data(), candidate.size}, round_trip);
+    if (report.result != TrackLoadResult::loaded) {
+        if (refusal != nullptr) {
+            *refusal = report;
+        }
         return TrackSerializeResult::invalid_definition;
     }
     output = candidate;

firmware/components/track/include/track_timer/track/definition.hpp

@@ -146,8 +146,18 @@ struct TrackLoadReport {
 
 [[nodiscard]] TrackLoadReport load_track_definition(std::string_view json,
                                                     TrackDefinition& output) noexcept;
+// Serialising validates: the candidate is written, read back through
+// load_track_definition, and refused if it does not survive the round trip. That is what
+// keeps an invalid definition from reaching the card, and it is why the reason is worth
+// having - pass `refusal` to receive the round trip's report rather than only the fact
+// that there was one. A capture refused at a circuit has to say which check failed,
+// because a line that is too short, one outside the geofence and one at too shallow an
+// angle to the direction of travel have nothing in common but the refusal.
+//
+// Only written when the round trip fails; a serialized definition leaves it untouched.
 [[nodiscard]] TrackSerializeResult serialize_track_definition(
-    const TrackDefinition& definition, TrackDefinitionBlob& output) noexcept;
+    const TrackDefinition& definition, TrackDefinitionBlob& output,
+    TrackLoadReport* refusal = nullptr) noexcept;
 [[nodiscard]] std::uint64_t hash_track_definition(std::string_view bytes) noexcept;
 void format_definition_hash(std::uint64_t hash, std::array<char, 17>& output) noexcept;
 [[nodiscard]] bool track_timing_ready(const TrackDefinition& definition) noexcept;

firmware/components/ui/gate_capture.cpp

@@ -11,7 +11,7 @@
 namespace track_timer::ui {
 namespace {
 
-constexpr std::array<const char*, 2> kGateNames{"START", "FINISH"};
+constexpr std::array<const char*, kGateCaptureGateCount> kGateNames{"START", "FINISH"};
 
 const char* status_text(const GateCaptureStatus status) noexcept
 {
@@ -21,7 +21,8 @@ const char* status_text(const GateCaptureStatus status) noexcept
     case GateCaptureStatus::editing:
         return "PARK SAFELY AT THE PHYSICAL ENDPOINT, THEN CAPTURE";
     case GateCaptureStatus::endpoint_captured:
-        return "ENDPOINT CAPTURED - CONTINUE UNTIL ALL 8 ARE COMPLETE";
+        // Written with the live count in write_status; this is the fallback.
+        return "ENDPOINT CAPTURED";
     case GateCaptureStatus::active_session:
         return "CAPTURE BLOCKED - STOP THE ACTIVE SESSION FIRST";
     case GateCaptureStatus::moving:
@@ -33,19 +34,77 @@ const char* status_text(const GateCaptureStatus status) noexcept
     case GateCaptureStatus::poor_accuracy:
         return "CAPTURE BLOCKED - GPS ACCURACY MUST BE 5 M OR BETTER";
     case GateCaptureStatus::incomplete:
-        return "SAVE BLOCKED - CAPTURE BOTH ENDS OF ALL FOUR GATES";
+        // The gates are named rather than counted: a count goes stale when the shape of a
+        // circuit changes, as this text did, and "START AND FINISH" is also the clearer
+        // instruction to somebody holding the device at the line.
+        return "SAVE BLOCKED - CAPTURE BOTH ENDS OF THE START AND FINISH LINES";
     case GateCaptureStatus::invalid_geometry:
         return "SAVE BLOCKED - GATE GEOMETRY FAILED SAFETY VALIDATION";
     case GateCaptureStatus::overwrite_confirmation:
         return "EXISTING JSON FOUND - PRESS CONFIRM OVERWRITE TO REPLACE IT";
     case GateCaptureStatus::saved:
-        return "SAVED AND RELOADED - FOUR-GATE DEFINITION IS READY";
+        return "SAVED AND RELOADED - START AND FINISH LINES ARE READY";
     case GateCaptureStatus::storage_error:
         return "SAVE FAILED - PREVIOUS VALID JSON REMAINS IN PLACE";
     }
     return "GATE CAPTURE";
 }
 
+// Why validation refused the captured geometry, in the terms of the thing to go and do
+// about it. Three results are worth translating because they are the three a capture can
+// actually produce; anything else is reported as the validator named it rather than
+// guessed at.
+void write_refusal(std::array<char, 144>& status,
+                   const track::TrackLoadReport& refusal) noexcept
+{
+    switch (refusal.result) {
+    case track::TrackLoadResult::degenerate_start_gate:
+    case track::TrackLoadResult::degenerate_finish_gate:
+        std::snprintf(status.data(), status.size(),
+                      "SAVE BLOCKED - LINE TOO SHORT: THE TWO ENDS MUST BE AT LEAST 1 M "
+                      "APART");
+        return;
+    case track::TrackLoadResult::geometry_outside_geofence:
+        std::snprintf(status.data(), status.size(),
+                      "SAVE BLOCKED - AN ENDPOINT IS OUTSIDE THIS CIRCUIT'S GEOFENCE");
+        return;
+    case track::TrackLoadResult::invalid_value:
+        // Three checks share this result for a captured line: the projection failing
+        // because the endpoints are nowhere near the circuit, a line over a kilometre
+        // long, and a line within ten degrees of the direction of travel. From here there
+        // is no telling which of them refused it, so all three are named rather than one
+        // being picked and presented as the cause.
+        std::snprintf(status.data(), status.size(),
+                      "SAVE BLOCKED - %s REFUSED. CHECK ITS LENGTH, ITS ANGLE TO THE "
+                      "DIRECTION OF TRAVEL, AND THAT IT IS AT THIS CIRCUIT",
+                      track::track_definition_field_name(refusal.error_field));
+        return;
+    default:
+        break;
+    }
+    std::snprintf(status.data(), status.size(), "SAVE BLOCKED - GEOMETRY REFUSED: %s",
+                  track::track_load_result_name(refusal.result));
+}
+
+// The status line. Counted states are formatted from kGateCaptureEndpointCount rather than
+// from a number typed into a string, which is how the eight-endpoint instruction outlived
+// the eight endpoints.
+void write_status(std::array<char, 144>& status, const GateCaptureStatus state,
+                  const std::size_t captured, const track::TrackLoadReport& refusal) noexcept
+{
+    if (state == GateCaptureStatus::endpoint_captured) {
+        std::snprintf(status.data(), status.size(),
+                      "ENDPOINT CAPTURED - %zu OF %zu DONE", captured,
+                      kGateCaptureEndpointCount);
+        return;
+    }
+    if (state == GateCaptureStatus::invalid_geometry) {
+        write_refusal(status, refusal);
+        return;
+    }
+    std::snprintf(status.data(), status.size(), "%s", status_text(state));
+}
+
 GateCaptureStatus from_endpoint_result(const track::EndpointCaptureResult result) noexcept
 {
     switch (result) {
@@ -87,6 +146,7 @@ void GateCaptureController::begin(const track::TrackDefinition& definition,
     }
     latest_fix_ = {};
     evaluation_monotonic_us_ = domain::kUnavailableTime;
+    refusal_ = {};
     captured_.fill(false);
     gate_index_ = 0;
     right_endpoint_ = false;
@@ -106,7 +166,7 @@ void GateCaptureController::previous_gate() noexcept
     if (status_ == GateCaptureStatus::closed) {
         return;
     }
-    gate_index_ = gate_index_ == 0 ? 1 : gate_index_ - 1;
+    gate_index_ = gate_index_ == 0 ? kGateCaptureGateCount - 1 : gate_index_ - 1;
     status_ = GateCaptureStatus::editing;
 }
 
@@ -115,7 +175,7 @@ void GateCaptureController::next_gate() noexcept
     if (status_ == GateCaptureStatus::closed) {
         return;
     }
-    gate_index_ = (gate_index_ + 1) % 2;
+    gate_index_ = (gate_index_ + 1) % kGateCaptureGateCount;
     status_ = GateCaptureStatus::editing;
 }
 
@@ -171,8 +231,17 @@ void GateCaptureController::save(const bool confirm_overwrite) noexcept
     candidate.provenance.geometry_status =
         track::TrackGeometryStatus::device_captured;
     track::TrackDefinitionBlob blob{};
-    if (track::serialize_track_definition(candidate, blob) !=
-        track::TrackSerializeResult::serialized) {
+    refusal_ = {};
+    const auto serialized =
+        track::serialize_track_definition(candidate, blob, &refusal_);
+    if (serialized != track::TrackSerializeResult::serialized) {
+        // The round trip leaves its report; a blob that would not fit at all leaves none,
+        // so the one result that says so is set here rather than reported as an empty
+        // definition the validator never saw.
+        if (serialized == track::TrackSerializeResult::capacity_exceeded) {
+            refusal_ = {track::TrackLoadResult::capacity_exceeded, 0,
+                        track::TrackDefinitionField::none};
+        }
         status_ = GateCaptureStatus::invalid_geometry;
         return;
     }
@@ -226,14 +295,19 @@ const track::TrackDefinition& GateCaptureController::draft() const noexcept
     return draft_;
 }
 
+const track::TrackLoadReport& GateCaptureController::refusal() const noexcept
+{
+    return refusal_;
+}
+
 GateCaptureViewModel GateCaptureController::view_model() const noexcept
 {
     GateCaptureViewModel model{};
     std::snprintf(model.track.data(), model.track.size(), "%.44s | REV %u",
                   draft_.name.data(), static_cast<unsigned>(draft_.revision));
-    std::snprintf(model.selection.data(), model.selection.size(), "%s | %s | %u/8",
+    std::snprintf(model.selection.data(), model.selection.size(), "%s | %s | %zu/%zu",
                   kGateNames[gate_index_], right_endpoint_ ? "RIGHT" : "LEFT",
-                  static_cast<unsigned>(captured_endpoint_count()));
+                  captured_endpoint_count(), kGateCaptureEndpointCount);
     std::snprintf(model.endpoint_label.data(), model.endpoint_label.size(), "%s ENDPOINT",
                   right_endpoint_ ? "RIGHT" : "LEFT");
     std::snprintf(model.save_label.data(), model.save_label.size(), "%s",
@@ -270,7 +344,7 @@ GateCaptureViewModel GateCaptureController::view_model() const noexcept
                       "LINE PREVIEW: LEFT %s | RIGHT %s",
                       left_done ? "OK" : "MISSING", right_done ? "OK" : "MISSING");
     }
-    std::snprintf(model.status.data(), model.status.size(), "%s", status_text(status_));
+    write_status(model.status, status_, captured_endpoint_count(), refusal_);
     model.status_color_rgb = status_ == GateCaptureStatus::saved ||
                                      status_ == GateCaptureStatus::endpoint_captured
                                  ? color::positive_bright

firmware/components/ui/include/track_timer/ui/gate_capture.hpp

@@ -9,6 +9,17 @@
 
 namespace track_timer::ui {
 
+// What a circuit is made of, stated once. Two directed gates, each with a left and a right
+// endpoint, so four endpoints in all.
+//
+// It was eight until pit entry and exit were removed (#227). The controller followed; the
+// text it hands the screen did not, so the device went on instructing the driver to capture
+// eight endpoints across four gates while save unlocked at four and two. Counting from
+// here, or naming the gates rather than counting them, is what keeps the next change to
+// this shape from leaving the same kind of lie on the panel.
+inline constexpr std::size_t kGateCaptureGateCount = 2;
+inline constexpr std::size_t kGateCaptureEndpointCount = kGateCaptureGateCount * 2;
+
 enum class GateCaptureStatus : std::uint8_t {
     closed,
     editing,
@@ -30,7 +41,10 @@ struct GateCaptureViewModel {
     std::array<char, 48> selection{};
     std::array<char, 96> fix{};
     std::array<char, 96> preview{};
-    std::array<char, 112> status{};
+    // Wide enough for the longest refusal: the status label is 560 px of caption text set
+    // to wrap, which is about 75 characters a line over two lines, and a geometry refusal
+    // has to name the gate and every check that could have produced it.
+    std::array<char, 144> status{};
     std::array<char, 24> endpoint_label{};
     std::array<char, 24> save_label{};
     std::uint32_t status_color_rgb{0xFFFFFF};
@@ -57,6 +71,8 @@ class GateCaptureController {
     [[nodiscard]] bool right_endpoint_selected() const noexcept;
     [[nodiscard]] std::size_t captured_endpoint_count() const noexcept;
     [[nodiscard]] const track::TrackDefinition& draft() const noexcept;
+    // Meaningful only while the status is invalid_geometry.
+    [[nodiscard]] const track::TrackLoadReport& refusal() const noexcept;
     [[nodiscard]] GateCaptureViewModel view_model() const noexcept;
 
   private:
@@ -69,7 +85,12 @@ class GateCaptureController {
     domain::GnssFix latest_fix_{};
     std::int64_t evaluation_monotonic_us_{domain::kUnavailableTime};
     // Two endpoints for each of the two gates that remain.
-    std::array<bool, 4> captured_{};
+    std::array<bool, kGateCaptureEndpointCount> captured_{};
+    // Why the last save was refused, when it was refused by the geometry rather than by
+    // the card. Kept because the three ways a captured line fails validation have three
+    // different remedies, and "GATE GEOMETRY FAILED SAFETY VALIDATION" names none of them
+    // to somebody standing at the circuit with the engine off.
+    track::TrackLoadReport refusal_{};
     std::size_t gate_index_{0};
     bool right_endpoint_{false};
     GateCaptureStatus status_{GateCaptureStatus::closed};

firmware/main/screen_router.cpp

@@ -1438,11 +1438,22 @@ class ScreenRouter {
         // The status rather than the action, because the status is the part that answers
         // why nothing happened: "moving" and "stale-fix" are refusals with different
         // causes, and from outside they look identical.
-        ESP_LOGI("track_timer", "capture: %s - gate %u %s, %u of 8 endpoints",
+        ESP_LOGI("track_timer", "capture: %s - gate %u %s, %u of %u endpoints",
                  ui::gate_capture_status_name(gate_capture_.status()),
                  static_cast<unsigned>(gate_capture_.gate_index()),
                  gate_capture_.right_endpoint_selected() ? "right" : "left",
-                 static_cast<unsigned>(gate_capture_.captured_endpoint_count()));
+                 static_cast<unsigned>(gate_capture_.captured_endpoint_count()),
+                 static_cast<unsigned>(ui::kGateCaptureEndpointCount));
+        // Which check refused the geometry, not just that one did. This is logged at the
+        // circuit, where the difference between a line too short, a line outside the
+        // geofence and a line too close to the direction of travel is the difference
+        // between walking ten metres and giving up on the session.
+        if (gate_capture_.status() == ui::GateCaptureStatus::invalid_geometry) {
+            const auto& refusal = gate_capture_.refusal();
+            ESP_LOGW("track_timer", "capture: geometry refused - %s at %s",
+                     track::track_load_result_name(refusal.result),
+                     track::track_definition_field_name(refusal.error_field));
+        }
         refresh_gate_capture();
     }
 

simulator/CMakeLists.txt

@@ -235,7 +235,7 @@ if(BUILD_TESTING)
     set_tests_properties(
         simulator-gate-capture
         PROPERTIES
-            PASS_REGULAR_EXPRESSION "Two-gate capture, safety rejection, atomic persistence, and recovery passed"
+            PASS_REGULAR_EXPRESSION "Two-gate capture, the instructions matching it, named geometry refusals, safety rejection, atomic persistence, and recovery passed"
             TIMEOUT 20
     )