Tests for the guards whose removal fails closed

test/test_commands/test_main.cpp

@@ -262,7 +262,21 @@ static void test_the_allowance_refills_after_a_quiet_period() {
         d.dispatch(cmd(InverterCommandType::SetActivePowerLimitPercent, 50.0), driver);
     }
     g_now += 2000;
-    TEST_ASSERT_EQUAL(CommandResult::Ok,
+
+    // THE WHOLE BURST must be back, not just one command. The limiter has two ways to say
+    // yes after a quiet period -- the burst refilling, and the separate "one through per
+    // minInterval" allowance -- and asserting a single Ok cannot tell them apart. It passed
+    // with the refill deleted, which would have shipped a burst of three that behaved like a
+    // burst of one, silently, on the path that asserts a DRM mode.
+    for (int i = 0; i < 3; ++i) {
+        TEST_ASSERT_EQUAL_MESSAGE(
+            CommandResult::Ok,
+            d.dispatch(cmd(InverterCommandType::SetActivePowerLimitPercent, 50.0), driver).result,
+            "the refilled allowance must be the full burst");
+    }
+    // And the fourth is refused again, so the refill restored the allowance rather than
+    // removing the limit.
+    TEST_ASSERT_EQUAL(CommandResult::RateLimited,
                       d.dispatch(cmd(InverterCommandType::SetActivePowerLimitPercent, 50.0), driver)
                           .result);
 }

test/test_config_backup/test_main.cpp

@@ -319,6 +319,21 @@ static void test_a_configuration_that_fails_validation_is_refused() {
 static void test_an_oversized_file_is_refused_before_parsing() {
     BackupContents contents;
     std::string    detail;
+
+    // VALID JSON, and too big. The fixture used to be a wall of 'x', which deserializeJson
+    // rejects on its own -- so deleting the size ceiling entirely left this green and the
+    // test could not tell "refused for being oversized" from "refused for not being JSON".
+    // The ceiling exists to stop a large well-formed document reaching the parser at all, on
+    // a device where that memory is not available to lose.
+    std::string big = R"({"format_version":1,"configuration":{"note":")";
+    big.append(kMaxBackupBytes, 'a');
+    big += R"("}})";
+    TEST_ASSERT_TRUE_MESSAGE(big.size() > kMaxBackupBytes, "the fixture must exceed the ceiling");
+    TEST_ASSERT_EQUAL(BackupResult::NotJson, parseConfigBackup(big, contents, detail));
+    TEST_ASSERT_TRUE_MESSAGE(detail.find("larger") != std::string::npos,
+                             "and must say it was the SIZE, not the syntax");
+
+    // The original case still holds: not-JSON is also refused, for its own reason.
     TEST_ASSERT_EQUAL(BackupResult::NotJson,
                       parseConfigBackup(std::string(kMaxBackupBytes + 1, 'x'), contents, detail));
 }

test/test_discovery/test_main.cpp

@@ -31,6 +31,9 @@ class FakeDriver : public InverterDriver {
         std::string model      = "Model-1";
         /// Second and later probes report this serial instead, simulating an unstable match.
         std::string serialOnRepeat;
+        /// The same, for the model. probesAgree() compares serial AND model, and only the
+        /// serial half was ever varied -- so dropping the model comparison changed nothing.
+        std::string modelOnRepeat;
         bool        writeAttempted = false;
         /// Non-zero: begin() reconfigures the line to this, as every real driver does.
         uint32_t    begunAtBaud = 0;
@@ -71,7 +74,9 @@ class FakeDriver : public InverterDriver {
         r.responded      = script_->responded;
         r.checksumValid  = script_->checksumValid;
         r.confidenceScore = script_->score;
-        r.detectedModel  = script_->model;
+        r.detectedModel  = (probeCount_ > 1 && !script_->modelOnRepeat.empty())
+                               ? script_->modelOnRepeat
+                               : script_->model;
         r.serialNumber = (probeCount_ > 1 && !script_->serialOnRepeat.empty())
                              ? script_->serialOnRepeat
                              : script_->serial;
@@ -127,6 +132,10 @@ struct AddressBus {
     std::vector<std::string> probed;
     /// Same serial at every address, for the one-device-two-addresses case.
     bool sharedSerial = false;
+    /// No serial at all, which several protocols genuinely cannot report. Two silent-serial
+    /// units are two units, and mergeDuplicateSerials() has a guard saying so that no fixture
+    /// ever reached.
+    bool emptySerial = false;
 };
 
 class AddressedDriver : public InverterDriver {
@@ -151,7 +160,9 @@ class AddressedDriver : public InverterDriver {
         r.checksumValid   = true;
         r.confidenceScore = 95;
         r.detectedModel   = "MIC-TL-X";
-        r.serialNumber    = bus_->sharedSerial ? "SHARED" : "SER-" + address_;
+        r.serialNumber    = bus_->emptySerial   ? std::string()
+                            : bus_->sharedSerial ? std::string("SHARED")
+                                                 : "SER-" + address_;
         return r;
     }
 
@@ -576,6 +587,131 @@ static void test_threshold_is_configurable() {
     TEST_ASSERT_TRUE(e.run(DiscoveryMode::Quick, c).autoSelected);
 }
 
+// probesAgree() compares responded, serial AND model. Only the serial half was ever varied by
+// a fixture, so deleting the model comparison left the suite green: a device naming a different
+// model on the second ask would have been called consistent and auto-selected.
+// Two descriptor guards that nothing reached. Both defend the wizard against a MALFORMED
+// driver declaration rather than against a misbehaving device, so no fixture built to describe
+// a device ever produced one -- which is precisely why they went unverified.
+
+// A numeric address option with no default cannot be swept: probesFor() puts the driver's own
+// default first and always includes it, so an empty one would sweep an address the driver never
+// declared. Removing the defaultValue requirement left the suite green.
+// The mirror image of the one-device-two-addresses case. mergeDuplicateSerials() folds
+// candidates that share a serial, and skips any candidate whose serial is EMPTY -- because
+// nothing can be matched on, so two silent units are two units. No fixture ever had two
+// responding candidates without a serial, so deleting that guard went unnoticed: it would have
+// collapsed two genuinely distinct inverters into one, and the owner would configure half a bus.
+static void test_two_devices_without_serial_numbers_are_not_folded_into_one() {
+    DriverRegistry reg;
+    MockTransport  t;
+    AddressBus     bus;
+    bus.occupied    = {"1", "2"};
+    bus.emptySerial = true;
+    addAddressedDriver(reg, addressedDesc("silent_serial", "1"), &bus);
+
+    DiscoveryEngine e(reg, t);
+    const auto      out = e.run(DiscoveryMode::Extended);
+
+    TEST_ASSERT_EQUAL_size_t(2, out.candidates.size());
+}
+
+static void test_an_address_option_without_a_default_is_not_sweepable() {
+    TEST_ASSERT_TRUE(addressedDesc("ok", "1").hasSweepableAddress());
+    TEST_ASSERT_FALSE_MESSAGE(addressedDesc("no-default", "").hasSweepableAddress(),
+                              "a numeric address option with no default cannot anchor a sweep");
+}
+
+// A default outside the option's OWN declared bounds. The guard's comment names the failure
+// exactly: such a value "would be probed, reported, offered by the wizard, and then refused by
+// the PATCH gate: a dead end". No driver in the suite declared one, so the bounds check was
+// validated by nothing.
+static void test_a_default_outside_its_own_bounds_is_refused_rather_than_offered() {
+    const DriverDescriptor inRange = addressedDesc("sane", "5", 1, 8);
+    long                   out     = 0;
+    TEST_ASSERT_TRUE(inRange.numericOption({}, "unit_id", out));
+    TEST_ASSERT_EQUAL_INT32(5, out);
+
+    // 10 is SolaX's real default and outside a 1..8 declaration -- the shape this guards.
+    const DriverDescriptor contradictory = addressedDesc("contradictory", "10", 1, 8);
+    out = 0;
+    TEST_ASSERT_FALSE_MESSAGE(contradictory.numericOption({}, "unit_id", out),
+                              "a default outside its own bounds is a dead end, not an address");
+    TEST_ASSERT_EQUAL_INT32(0, out);  // and nothing is written on refusal
+}
+
+static void test_a_device_that_names_a_different_model_on_the_second_ask_is_inconsistent() {
+    DriverRegistry     reg;
+    MockTransport      t;
+    FakeDriver::Script s;
+    s.score         = 100;
+    s.serial        = "SER-1";  // the serial AGREES; only the model moves
+    s.model         = "Model-A";
+    s.modelOnRepeat = "Model-B";
+    addDriver(reg, desc("shifty", 10), &s);
+
+    DiscoveryEngine e(reg, t);
+    const auto      out = e.run(DiscoveryMode::Quick);
+
+    TEST_ASSERT_FALSE(out.candidates[0].consistent);
+    TEST_ASSERT_EQUAL_INT(50, out.candidates[0].probe.confidenceScore);
+    TEST_ASSERT_FALSE(out.autoSelected);
+}
+
+// The consistency veto sits AFTER the confidence threshold, and the existing test scores 100 --
+// halved to 50, already below the threshold of 80. So the threshold did the blocking and the
+// veto itself was never exercised: removing it changed nothing. A score that survives halving
+// is what puts the veto on its own.
+static void test_the_consistency_veto_blocks_on_its_own_not_only_via_the_threshold() {
+    DriverRegistry     reg;
+    MockTransport      t;
+    FakeDriver::Script s;
+    s.score          = 200;  // halves to 100, comfortably above minConfidence
+    s.serial         = "SER-1";
+    s.serialOnRepeat = "SER-2";
+    addDriver(reg, desc("flaky", 10), &s);
+
+    DiscoveryEngine e(reg, t);
+    const auto      out = e.run(DiscoveryMode::Quick);
+
+    TEST_ASSERT_EQUAL_INT(100, out.candidates[0].probe.confidenceScore);
+    TEST_ASSERT_FALSE_MESSAGE(out.autoSelected, "an inconsistent probe must not be selected");
+    TEST_ASSERT_TRUE(out.reason.find("disagreed") != std::string::npos);
+
+    // The same candidate WITH the veto disabled is selected -- which is what proves the veto
+    // did the blocking, and not the threshold or the margin.
+    DriverRegistry     reg2;
+    MockTransport      t2;
+    FakeDriver::Script s2 = s;
+    addDriver(reg2, desc("flaky", 10), &s2);
+    DiscoveryConfig permissive;
+    permissive.requireConsistentProbes = false;
+    DiscoveryEngine e2(reg2, t2);
+    TEST_ASSERT_TRUE(e2.run(DiscoveryMode::Quick, permissive).autoSelected);
+}
+
+// mergeDuplicateSerials() folds two candidates together only when the DRIVER ID matches as well
+// as the serial. No fixture ever had two different drivers reporting the same serial, so
+// dropping the id comparison went unnoticed -- and it would have silently deleted one of two
+// genuinely different protocol candidates, which is exactly the disambiguation the user needs.
+static void test_two_different_drivers_reporting_one_serial_stay_two_candidates() {
+    DriverRegistry     reg;
+    MockTransport      t;
+    FakeDriver::Script a;
+    FakeDriver::Script b;
+    a.score  = 90;
+    b.score  = 85;
+    a.serial = "SAME-SERIAL";
+    b.serial = "SAME-SERIAL";
+    addDriver(reg, desc("driver-a", 10), &a);
+    addDriver(reg, desc("driver-b", 20), &b);
+
+    DiscoveryEngine e(reg, t);
+    const auto      out = e.run(DiscoveryMode::Quick);
+
+    TEST_ASSERT_EQUAL_size_t(2, out.candidates.size());
+}
+
 static void test_inconsistent_probes_halve_the_score_and_block_selection() {
     // A device that identifies as something different on the second ask was never identified.
     DriverRegistry     reg;
@@ -882,6 +1018,12 @@ int main(int, char**) {
     RUN_TEST(test_a_score_below_the_threshold_is_never_auto_selected);
     RUN_TEST(test_threshold_is_configurable);
     RUN_TEST(test_inconsistent_probes_halve_the_score_and_block_selection);
+    RUN_TEST(test_two_devices_without_serial_numbers_are_not_folded_into_one);
+    RUN_TEST(test_an_address_option_without_a_default_is_not_sweepable);
+    RUN_TEST(test_a_default_outside_its_own_bounds_is_refused_rather_than_offered);
+    RUN_TEST(test_a_device_that_names_a_different_model_on_the_second_ask_is_inconsistent);
+    RUN_TEST(test_the_consistency_veto_blocks_on_its_own_not_only_via_the_threshold);
+    RUN_TEST(test_two_different_drivers_reporting_one_serial_stay_two_candidates);
     RUN_TEST(test_consistent_probes_are_recorded_as_evidence);
     RUN_TEST(test_a_failed_checksum_blocks_selection);
     RUN_TEST(test_a_silent_bus_yields_no_candidates_and_a_useful_reason);

test/test_maxtalk/test_main.cpp

@@ -230,6 +230,69 @@ static void test_frame_length_finds_the_boundary_in_a_longer_buffer() {
     TEST_ASSERT_EQUAL_size_t(n, maxtalk::frameLength(buf, n + 7));
 }
 
+/// Builds an otherwise-perfect reply -- correct declared length, correct checksum -- around a
+/// caller-chosen marker and payload. Both callers below need to vary exactly one thing and keep
+/// every other check satisfied, or the parser refuses the frame before reaching what is under
+/// test. That is the failure this suite has already been bitten by once.
+static size_t buildReply(const char* marker, const char* payload, char* frame) {
+    const size_t total = 9 + (std::strlen(marker) + std::strlen(payload) + 1) + 5;
+    int          w     = 0;
+    frame[w++] = '{';
+    frame[w++] = '0'; frame[w++] = '5'; frame[w++] = ';';
+    frame[w++] = 'F'; frame[w++] = 'B'; frame[w++] = ';';
+    frame[w++] = static_cast<char>(total / 16 < 10 ? '0' + total / 16 : 'A' + total / 16 - 10);
+    frame[w++] = static_cast<char>(total % 16 < 10 ? '0' + total % 16 : 'A' + total % 16 - 10);
+    std::memcpy(frame + w, marker, std::strlen(marker));
+    w += static_cast<int>(std::strlen(marker));
+    std::memcpy(frame + w, payload, std::strlen(payload));
+    w += static_cast<int>(std::strlen(payload));
+    frame[w++] = '|';
+    const uint16_t sum = maxtalk::checksum(frame + 1, static_cast<size_t>(w) - 1);
+    for (int i = 0; i < 4; ++i) {
+        const uint8_t nib = (sum >> (12 - 4 * i)) & 0xF;
+        frame[w++] = static_cast<char>(nib < 10 ? '0' + nib : 'A' + nib - 10);
+    }
+    frame[w++] = '}';
+    return static_cast<size_t>(w);
+}
+
+// No test ever corrupted the "|64:" marker, so deleting the memcmp that checks it left the
+// suite green. A frame whose marker is garbled or shifted by a byte would then be parsed from
+// the wrong offset -- and everything after it is read as code=value pairs, so the failure is
+// not "no reading" but plausible readings taken from the wrong place in the frame.
+static void test_a_frame_whose_payload_marker_is_wrong_is_refused() {
+    const char* markers[] = {"|65:", "|64;", "@64:", "|640", "::::"};
+    for (const char* m : markers) {
+        char         frame[maxtalk::kMaxFrame];
+        const size_t n = buildReply(m, "CAC=1F3E", frame);
+
+        maxtalk::Reading readings[4];
+        size_t           count = 0;
+        TEST_ASSERT_EQUAL_MESSAGE(maxtalk::ParseResult::Malformed,
+                                  maxtalk::parseReply(frame, n, 0x05, readings, 4, count), m);
+    }
+}
+
+// The codec writes uppercase, so every fixture in this suite is uppercase and the lowercase
+// branch of hexValue() was never executed -- deleting it changed nothing. The vendor is gone
+// and there is no second implementation to compare against, so "our own encoder never emits
+// it" is not evidence about what a device emits. The branch exists; this is what it claims.
+static void test_a_lowercase_hex_value_decodes_the_same_as_uppercase() {
+    char         upper[maxtalk::kMaxFrame];
+    char         lower[maxtalk::kMaxFrame];
+    const size_t nu = buildReply("|64:", "CAC=1F3E", upper);
+    const size_t nl = buildReply("|64:", "CAC=1f3e", lower);
+
+    maxtalk::Reading ru[4], rl[4];
+    size_t           cu = 0, cl = 0;
+    TEST_ASSERT_EQUAL(maxtalk::ParseResult::Ok, maxtalk::parseReply(upper, nu, 0x05, ru, 4, cu));
+    TEST_ASSERT_EQUAL(maxtalk::ParseResult::Ok, maxtalk::parseReply(lower, nl, 0x05, rl, 4, cl));
+    TEST_ASSERT_EQUAL_size_t(1, cu);
+    TEST_ASSERT_EQUAL_size_t(1, cl);
+    TEST_ASSERT_EQUAL_UINT32(0x1F3E, ru[0].value);
+    TEST_ASSERT_EQUAL_UINT32(ru[0].value, rl[0].value);
+}
+
 static void test_a_malformed_payload_is_refused_rather_than_half_decoded() {
     // The frame must be otherwise PERFECT -- correct length, correct checksum -- or the parser
     // rejects it earlier and the payload logic is never reached. An earlier version of this test
@@ -338,6 +401,8 @@ int main() {
     RUN_TEST(test_an_unterminated_frame_asks_for_more_bytes);
     RUN_TEST(test_frame_length_finds_the_boundary_in_a_longer_buffer);
     RUN_TEST(test_a_malformed_payload_is_refused_rather_than_half_decoded);
+    RUN_TEST(test_a_frame_whose_payload_marker_is_wrong_is_refused);
+    RUN_TEST(test_a_lowercase_hex_value_decodes_the_same_as_uppercase);
     RUN_TEST(test_running_out_of_room_keeps_what_was_already_decoded);
     RUN_TEST(test_a_request_that_does_not_fit_is_refused_rather_than_truncated);
     RUN_TEST(test_a_request_is_refused_for_codes_that_cannot_exist);

test/test_solarmax/test_main.cpp

@@ -235,6 +235,32 @@ static void test_an_unusable_reply_leaves_the_state_completely_untouched() {
     TEST_ASSERT_EQUAL_STRING("untouched", s.statusText.c_str());
 }
 
+// Every other test in this suite reads capabilities and identity off the DRIVER (d.capabilities(),
+// d.identity()). Nothing read them off the polled DeviceState -- which is the copy REST, MQTT,
+// Prometheus and the Modbus register map all publish from. Deleting both assignments at the end
+// of poll() therefore left the whole suite green, while a bridge would have reported a device
+// with no manufacturer, no protocol name and no capabilities at all.
+static void test_a_poll_publishes_the_identity_and_capabilities_into_the_state() {
+    MockTransport            t;
+    solarmax::SolarmaxDriver d(t, solarmax::SolarmaxOptions{0x05});
+    d.begin(t);
+    respondWith(t, 0x05, "PAC=1F4;UL1=8FC;IL1=64;TNF=1388;KDY=1F;KT0=3E8");
+
+    DeviceState s;
+    TEST_ASSERT_EQUAL(PollResult::Ok, d.poll(s));
+
+    TEST_ASSERT_EQUAL_STRING("SolarMax", s.identity.manufacturer.c_str());
+    TEST_ASSERT_EQUAL_STRING("MaxTalk RS485", s.identity.protocolName.c_str());
+    TEST_ASSERT_EQUAL_STRING(d.identity().driverId.c_str(), s.identity.driverId.c_str());
+
+    TEST_ASSERT_TRUE(s.capabilities.has(InverterCapability::ReadAcPower));
+    TEST_ASSERT_EQUAL_UINT8(d.capabilities().phaseCount, s.capabilities.phaseCount);
+    TEST_ASSERT_EQUAL_UINT8(d.capabilities().mpptCount, s.capabilities.mpptCount);
+    // Read-only is what every output keys its control surface off, so it has to survive the
+    // copy rather than be inferred from the driver id downstream.
+    TEST_ASSERT_TRUE(s.capabilities.isReadOnly());
+}
+
 static void test_the_probe_identifies_without_writing_anything_but_a_query() {
     MockTransport t;
     solarmax::SolarmaxDriver d(t, solarmax::SolarmaxOptions{0x05});
@@ -245,7 +271,13 @@ static void test_the_probe_identifies_without_writing_anything_but_a_query() {
     TEST_ASSERT_TRUE(r.responded);
     TEST_ASSERT_TRUE(r.checksumValid);
     TEST_ASSERT_EQUAL_STRING("SolarMax", r.detectedManufacturer.c_str());
-    TEST_ASSERT_TRUE(r.confidenceScore > 0);
+    // The exact score, not merely "some confidence". Discovery ranks candidate drivers against
+    // each other by this number, so it is a position on a scale rather than a flag: 100 is a
+    // driver that read back a serial number (EverSolar, SolaX), 95 a SunSpec device that
+    // answered its identity block, 40 an ambiguous SunSpec match, 30 the mock. 70 says "the
+    // protocol answered and named a type, but this family has no serial to confirm it with".
+    // Asserting > 0 accepted any of those, and the ranking is the whole point.
+    TEST_ASSERT_EQUAL_INT(70, r.confidenceScore);
     // Hex on the wire, hex in the label: "type 0x64", not the decimal 100.
     TEST_ASSERT_EQUAL_STRING("type 0x64", r.detectedModel.c_str());
 
@@ -315,6 +347,7 @@ int main() {
     RUN_TEST(test_a_reply_with_no_mapped_codes_is_not_a_successful_poll);
     RUN_TEST(test_an_unusable_reply_leaves_the_state_completely_untouched);
     RUN_TEST(test_the_probe_identifies_without_writing_anything_but_a_query);
+    RUN_TEST(test_a_poll_publishes_the_identity_and_capabilities_into_the_state);
     RUN_TEST(test_a_silent_bus_probes_as_no_device_rather_than_a_broken_one);
     RUN_TEST(test_another_devices_frame_during_a_probe_is_recorded_as_traffic);
     RUN_TEST(test_the_driver_refuses_every_write);