diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ba3e2a..ad76631 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/firmware/components/track/definition.cpp b/firmware/components/track/definition.cpp index c7f1f97..30b114a 100644 --- a/firmware/components/track/definition.cpp +++ b/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; diff --git a/firmware/components/track/include/track_timer/track/definition.hpp b/firmware/components/track/include/track_timer/track/definition.hpp index 68d295c..168439d 100644 --- a/firmware/components/track/include/track_timer/track/definition.hpp +++ b/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& output) noexcept; [[nodiscard]] bool track_timing_ready(const TrackDefinition& definition) noexcept; diff --git a/firmware/components/ui/gate_capture.cpp b/firmware/components/ui/gate_capture.cpp index 94d5b7f..2cf0396 100644 --- a/firmware/components/ui/gate_capture.cpp +++ b/firmware/components/ui/gate_capture.cpp @@ -11,7 +11,7 @@ namespace track_timer::ui { namespace { -constexpr std::array kGateNames{"START", "FINISH"}; +constexpr std::array 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& 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& 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(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(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 diff --git a/firmware/components/ui/include/track_timer/ui/gate_capture.hpp b/firmware/components/ui/include/track_timer/ui/gate_capture.hpp index 923034f..958438d 100644 --- a/firmware/components/ui/include/track_timer/ui/gate_capture.hpp +++ b/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 selection{}; std::array fix{}; std::array preview{}; - std::array 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 status{}; std::array endpoint_label{}; std::array 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 captured_{}; + std::array 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}; diff --git a/firmware/main/screen_router.cpp b/firmware/main/screen_router.cpp index 450d68e..bbee945 100644 --- a/firmware/main/screen_router.cpp +++ b/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(gate_capture_.gate_index()), gate_capture_.right_endpoint_selected() ? "right" : "left", - static_cast(gate_capture_.captured_endpoint_count())); + static_cast(gate_capture_.captured_endpoint_count()), + static_cast(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(); } diff --git a/simulator/CMakeLists.txt b/simulator/CMakeLists.txt index d2ba47e..92bcc08 100644 --- a/simulator/CMakeLists.txt +++ b/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 ) diff --git a/tests/cpp/test_gate_capture.cpp b/tests/cpp/test_gate_capture.cpp index f330454..9a1269e 100644 --- a/tests/cpp/test_gate_capture.cpp +++ b/tests/cpp/test_gate_capture.cpp @@ -70,8 +70,9 @@ track_timer::domain::GnssFix fix_at( void capture_all(track_timer::ui::GateCaptureController& controller, const track_timer::track::TrackDefinition& source) { - const std::array gates{ - source.gates.start, source.gates.finish}; + const std::array + gates{source.gates.start, source.gates.finish}; std::uint32_t sequence = 10; for (std::size_t gate_index = 0; gate_index < gates.size(); ++gate_index) { auto left = fix_at(gates[gate_index].left, sequence++); @@ -128,12 +129,37 @@ int main() controller.capture(false); assert(controller.status() == ui::GateCaptureStatus::poor_accuracy); + // What the screen tells the driver to do has to match what the controller will accept. + // It did not: the status asked for eight endpoints across four gates while save + // unlocked at four across two, because the pit gates went (#227) and this text did not + // (#204). Asserted against the stated count rather than a number typed in here, so the + // next change to the shape of a circuit cannot pass this test with stale instructions. + controller.begin(source, &store, false); + auto empty = controller.view_model(); + assert(!empty.can_save); + controller.save(false); + assert(controller.status() == ui::GateCaptureStatus::incomplete); + empty = controller.view_model(); + assert(std::strstr(empty.status.data(), "START AND FINISH") != nullptr); + assert(std::strstr(empty.status.data(), "FOUR GATES") == nullptr); + assert(std::strstr(empty.selection.data(), "0/4") != nullptr); + + controller.begin(source, &store, false); + auto first = fix_at(source.gates.start.left, 7); + controller.update_fix(first, first.arrival_monotonic_us + 10'000); + controller.capture(false); + assert(controller.status() == ui::GateCaptureStatus::endpoint_captured); + const auto after_one = controller.view_model(); + assert(std::strstr(after_one.status.data(), "1 OF 4 DONE") != nullptr); + assert(std::strstr(after_one.status.data(), "8") == nullptr); + controller.begin(source, &store, false); capture_all(controller, source); - assert(controller.captured_endpoint_count() == 4); + assert(controller.captured_endpoint_count() == ui::kGateCaptureEndpointCount); auto model = controller.view_model(); assert(model.can_save); assert(std::strstr(model.preview.data(), "LINE READY") != nullptr); + assert(std::strstr(model.selection.data(), "4/4") != nullptr); controller.save(false); assert(controller.status() == ui::GateCaptureStatus::saved); assert(controller.draft().revision == source.revision + 1); @@ -160,19 +186,56 @@ int main() assert(controller.status() == ui::GateCaptureStatus::storage_error); store.fail_write = false; + // Every endpoint taken at the same spot: both lines are a point, which validation + // refuses as degenerate. The refusal has to reach the panel as the thing to go and do + // about it - the three ways a captured line fails have three different remedies, and + // "GATE GEOMETRY FAILED SAFETY VALIDATION" names none of them to somebody standing at + // the circuit with the engine off (#204). controller.begin(source, &store, false); const auto writes_before_invalid = store.write_count; - for (int endpoint = 0; endpoint < 8; ++endpoint) { + for (std::size_t endpoint = 0; endpoint < ui::kGateCaptureEndpointCount; ++endpoint) { controller.update_fix(good, good.arrival_monotonic_us); controller.capture(false); controller.toggle_endpoint(); - if (endpoint % 2 == 1 && endpoint != 7) { + if (endpoint % 2 == 1 && endpoint + 1 < ui::kGateCaptureEndpointCount) { controller.next_gate(); } } controller.save(true); assert(controller.status() == ui::GateCaptureStatus::invalid_geometry); + assert(controller.refusal().result == track::TrackLoadResult::degenerate_start_gate); + const auto refused = controller.view_model(); + assert(std::strstr(refused.status.data(), "LINE TOO SHORT") != nullptr); + assert(std::strstr(refused.status.data(), "SAFETY VALIDATION") == nullptr); assert(store.write_count == writes_before_invalid); + + // The refusal a bench test produces, and the reason this is checkable away from a + // circuit at all: a line captured in the garden is a valid line in the wrong place. + // Both ends of both gates are shifted a few kilometres north, keeping each line its + // original length, so nothing is degenerate and the geofence is what refuses it. + { + auto distant = source; + constexpr double kNorthDeg = 0.05; + distant.gates.start.left.latitude_deg += kNorthDeg; + distant.gates.start.right.latitude_deg += kNorthDeg; + distant.gates.finish.left.latitude_deg += kNorthDeg; + distant.gates.finish.right.latitude_deg += kNorthDeg; + controller.begin(source, &store, false); + capture_all(controller, distant); + const auto writes_before = store.write_count; + controller.save(true); + assert(controller.status() == ui::GateCaptureStatus::invalid_geometry); + assert(controller.refusal().result == + track::TrackLoadResult::geometry_outside_geofence); + const auto outside = controller.view_model(); + assert(std::strstr(outside.status.data(), "OUTSIDE THIS CIRCUIT'S GEOFENCE") != + nullptr); + assert(store.write_count == writes_before); + } + + // And the reason does not outlive the attempt it belongs to. + controller.begin(source, &store, false); + assert(controller.refusal().result == track::TrackLoadResult::empty); controller.cancel(); assert(controller.status() == ui::GateCaptureStatus::closed); @@ -222,6 +285,7 @@ int main() assert(std::strlen(ui::gate_capture_status_name(status)) > 0); } - std::cout << "Two-gate capture, safety rejection, atomic persistence, and recovery passed\n"; + std::cout << "Two-gate capture, the instructions matching it, named geometry " + "refusals, safety rejection, atomic persistence, and recovery passed\n"; return 0; }