Fix Module copy assignment

[robekl]

Summary

• fix Module::operator= so it copies from the source object into the destination
• stop assignment from mutating the source object's spiConfig
• copy RF switch state and interrupt timing state as part of assignment
• add a regression test covering Module copy assignment behavior

Why this is a bug

The original Module::operator= copied this->spiConfig into mod.spiConfig instead of copying from mod into this.

That had two bad effects:

• the destination object did not receive the source SPI configuration
• the source object was silently mutated by assignment

The original operator also did not copy the RF switch state, so assignment produced an incomplete Module copy even aside from the reversed spiConfig copy.

History

Module copy constructor and assignment were introduced in commit b8de06288 ("Added copy ctor and assignment operator", July 5, 2020). The bad copy direction in operator= dates back to that introduction.

I did not find a later mitigation elsewhere in-tree. In-tree usage of Module assignment appears to be rare, which likely kept this bug latent, but the operator is public API and its current behavior is incorrect.

Validation

I ran the full existing unit suite in extras/test/unit with Homebrew GCC/Boost/fmt paths configured.

Against original code

I copied only the new regression test into a clean worktree at the original HEAD and reran the full suite. The new test failed, showing that:

• the assigned object did not receive RF switch state
• the source object's spiConfig was overwritten by assignment

Against this fix

I ran the full unit suite after the fix.

Result:

• *** No errors detected

Test coverage

Adds Module_copy_assignment_copies_spi_config_without_mutating_source in:

• extras/test/unit/tests/TestModule.cpp

The test verifies:

• destination receives spiConfig
• destination receives RF switch state
• source object is not mutated by assignment

[jgromes]

While I agree that this is indeed a bug, honest question, why do you feel the need to generate a novel-length wall of text for it? Seriously, the only problem was that the assignment operator implementation was missing some of the members. This happened because as these new member variables were added to the Module class, they were not added to the operator.

The provided test does not guard this in the future BTW - if we add some member variable to the Module class and not add it to the test, it will happily pass as it is not checking the newly added variable. So why is the test here?

[jgromes]

No human in the history of the world would ever name a test case like this. Why is Module_copy_assignment insufficient?

[jgromes]

If you really want to ensure the source is unchanged, you have to set different pins and check them.

[jgromes]

Why is size of this copy not checked against Module::RFSWITCH_MAX_PINS?

[jgromes]

Why are you reusing the module fixture to do this? It seems prone to breaking in the future if the approach to its setup/teardown changes. Especially since mod->rfSwitchTable is pointing a an arbitrary memory address.