diff --git a/test/test_commands/test_main.cpp b/test/test_commands/test_main.cpp index aca2cee..2cb850b 100644 --- a/test/test_commands/test_main.cpp +++ b/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); } diff --git a/test/test_config_backup/test_main.cpp b/test/test_config_backup/test_main.cpp index ea7b182..a772d3d 100644 --- a/test/test_config_backup/test_main.cpp +++ b/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)); } diff --git a/test/test_discovery/test_main.cpp b/test/test_discovery/test_main.cpp index 40f2f38..441c7b5 100644 --- a/test/test_discovery/test_main.cpp +++ b/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 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); diff --git a/test/test_maxtalk/test_main.cpp b/test/test_maxtalk/test_main.cpp index 2b6a0bc..ea6f941 100644 --- a/test/test_maxtalk/test_main.cpp +++ b/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(total / 16 < 10 ? '0' + total / 16 : 'A' + total / 16 - 10); + frame[w++] = static_cast(total % 16 < 10 ? '0' + total % 16 : 'A' + total % 16 - 10); + std::memcpy(frame + w, marker, std::strlen(marker)); + w += static_cast(std::strlen(marker)); + std::memcpy(frame + w, payload, std::strlen(payload)); + w += static_cast(std::strlen(payload)); + frame[w++] = '|'; + const uint16_t sum = maxtalk::checksum(frame + 1, static_cast(w) - 1); + for (int i = 0; i < 4; ++i) { + const uint8_t nib = (sum >> (12 - 4 * i)) & 0xF; + frame[w++] = static_cast(nib < 10 ? '0' + nib : 'A' + nib - 10); + } + frame[w++] = '}'; + return static_cast(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); diff --git a/test/test_solarmax/test_main.cpp b/test/test_solarmax/test_main.cpp index 99fe407..9e5539e 100644 --- a/test/test_solarmax/test_main.cpp +++ b/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);