V2 Beacon Functionality #240
Conversation
… health pre-repo organization to work with new repo structure)
… for boot count and flags
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… into new-state-of-health
…nces to flags/counters
There was a problem hiding this comment.
Initially I had developed a complex system of getting the name for a flag or counter from the associated registers. This worked well enough but then I asked myself... what if the builder of the proveskit wanted to add their own registers? Having them hardcoded here is not sufficiently flexible for the kit so we moved to having a get_name() method on flags and counters instead. Not perfect but OK for now.
There was a problem hiding this comment.
I'm sorry for lumping this in here but as I began removing unused and hardcoded flag and counters from satellite I noticed that Satellite wasn't doing very much anymore. I moved two methods over to the functions class and was able to delete everything else.
| gc.collect() | ||
| # all should be off from cubesat powermode | ||
|
|
||
| self.cubesat.f_softboot.toggle(True) |
There was a problem hiding this comment.
f_softboot had no functionality, it was being set here and then reset to false in the Satellite constructor.
There was a problem hiding this comment.
Just for new context on this, the f_softboot flag is there so the satellite can check on startup whether or not it is restarting gracefully or if it is coming back from a hard error (i.e. the watchdog forced a reset due to a software hang or other functional interrupt).
We right now are indeed not using it for anything, but in the future would probably want to bring it back somehow so the satellite can evaluate on reset whether it is fine to continue business as usual or if it suffered a critical fault and should enter a safe mode. There may be some interesting things to play around with here with the supervisor module as well.
nateinaction
changed the title
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors core flight software components to introduce a flexible beacon service, overhaul packet handling, and rewrite command processing using packetized messaging.
- Introduces a new
Beaconclass for extensible telemetry via variadic sensor inputs. - Moves and refactors packet logic into
hardware/radio/packetizer/PacketManagerto support arbitrary message sizes. - Rewrites
CommandDataHandlerto parse JSON commands over packets and handle them via a clean interface.
Reviewed Changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pysquared/beacon.py | New Beacon service that aggregates and sends sensor data as JSON. |
| pysquared/hardware/radio/packetizer/packet_manager.py | New packet manager for splitting, sending, and reassembling large payloads. |
| pysquared/cdh.py | CommandDataHandler overhaul: listens for JSON, validates, dispatches commands. |
| pysquared/sleep_helper.py | Refactored safe_sleep implementation with watchdog pets. |
| pysquared/hardware/radio/manager/**/* | Updated radio manager classes to simplify send logic and standardize APIs. |
Comments suppressed due to low confidence (4)
pysquared/hardware/radio/manager/base.py:54
- The docstring for
sendstill references license headers and payload assembly, but the implementation now just forwards raw bytes. Please update the docstring to accurately describe the current behavior.
def send(self, data: bytes) -> bool:
pysquared/hardware/radio/manager/rfm9x.py:83
- After validating the key, any parameter that isn't explicitly handled is silently ignored. Consider raising a
KeyErroror logging a warning for unsupportedkeyvalues to avoid silent no-ops.
def modify_config(self, key: str, value) -> None:
pysquared/beacon.py:114
- [nitpick] The
avg_readingsmethod bails out on the firstNonereturn but we lack tests for scenarios wherefuncreturns values thenNone. Adding a test for mixed successful/Nonereadings would improve coverage.
def avg_readings(
pysquared/sleep_helper.py:37
- [nitpick] The loop calls
time.monotonic()multiple times per iteration, which can drift if the clock advances. Cache the current time once per loop to compute remaining sleep time more predictably.
if duration > self.config.longest_allowable_sleep_time:
| assert manager._radio == mock_lora_instance | ||
| # Check high SF optimization IS set | ||
| assert mock_lora_instance.preamble_length == 10 | ||
| assert mock_lora_instance.low_datarate_optimize == 1 |
There was a problem hiding this comment.
Is there a reason that we are not checking this assert statement anymore? We do actually want to ideally ensure that low_datarate_optimize is being successfully set for spreading factors of 10 or higher.
The spreading_factor of LoRa radios is one of the primary driving factors behind the radio's overall data rate. The attached image is an example of what it looks like for the LoRaWAN protocol. Lower data rates generally mean better signal reception. When the data rate starts getting really low flipping the register for low_datarate_optimize plays a major role in ensuring packets don't get dropped.
There was a problem hiding this comment.
Yes, I intentionally removed this! When I looked through the Adafruit RFM library I found that the spreading_factor setter was managing this for us. https://github.com/adafruit/Adafruit_CircuitPython_RFM/blob/e242320b57317beb11b28676f9440593fbbfbfe1/adafruit_rfm/rfm9x.py#L481-L485
Mikefly123
left a comment
Mikefly123
left a comment
There was a problem hiding this comment.
Great work on this Nate! I think it overall looks great to me. I have just left a few comments on things to think about.
Nothing game breaking, so I think we'd be happy to go ahead and merge this stuff in and revisit a few of the questions in future pull requests!
| self._logger.debug( | ||
| "Received all expected packets", received=total_packets | ||
| ) | ||
| break |
There was a problem hiding this comment.
It might be a good idea for now to implement an else: statement here with a logger warning if not all packets were successfully received. I am presuming that in a future update we will add back in the retransmit request protocol for grabbing missed packets.
There was a problem hiding this comment.
The problem with putting an else here is that we can't know that we won't receive more packets after this. Since to see this message you will already be in DEBUG and see the individual packets come in with the debug line on 121.
| gc.collect() | ||
| # all should be off from cubesat powermode | ||
|
|
||
| self.cubesat.f_softboot.toggle(True) |
There was a problem hiding this comment.
Just for new context on this, the f_softboot flag is there so the satellite can check on startup whether or not it is restarting gracefully or if it is coming back from a hard error (i.e. the watchdog forced a reset due to a software hang or other functional interrupt).
We right now are indeed not using it for anything, but in the future would probably want to bring it back somehow so the satellite can evaluate on reset whether it is fine to continue business as usual or if it suffered a critical fault and should enter a safe mode. There may be some interesting things to play around with here with the supervisor module as well.
|
|
||
| self._radio.send(data=str(eval(args))) | ||
|
|
||
| def exec_cmd(self, cubesat: Satellite, args: str) -> None: |
There was a problem hiding this comment.
Although I can see how it makes sense to remove query and exec for safety reasons, I think it would be a good idea to plan to return some kind of functionality along these lines in the near future as arbitrary code execution is a nice little "escape hatch" to have access to for emergency updates to the software over the air.
Maybe we can open a ticket to look back into this at some future time.
There was a problem hiding this comment.
This made me think that individual satellite teams may want to be able to add their own commands. I think we need a new design for CDH that allows teams to expand what it's capable of doing on their satellite. Something like being able to provide a list of commands you want to have on your satellite and associating a function that will fire when those commands are received.
cdh(existing args, ("my_special_command", function), ...)
Summary
This PR began as a new "state of health" system, but evolved into a major refactor primarily centered on a new, more flexible beaconing system. During development, increased radio packet sizes required a refactor of the Packet Manager, which in turn led to a broader adoption of packetized messaging throughout the board codebases, requiring a Command Data Handler rewrite to maintain the ability to command the satellite. Ground station code has now been moved out into its own repository.
Breaking Changes:
SatelliteandFunctionsclasses. Downstream code using these will need to migrate to the new interfaces.Notable Features
Beacon
Beaconservice that accepts and outputs details for any number of supported sensors or hardware.Packet Manager
Command Data Handler Refactor
{ "password": "abc123", "command": "mycmd", "args": ["arg1", "arg2"] }Ground Station
Makefilefor the v4 board (PR). If we like this change, it can be ported to v5.Logging Changes
Other Changes
pyproject.toml(clerical only, no impact on developer workflow).config.jsonfiles across repos.SleepHelperbecause it was the only file left without testsHow was this tested?
Receiving Beacon
Sending joke request
Sending reset request
Sending change modulation request (⚠️ read-only OS, doesn't work atm)
Known Issues / Future Work
How to Review
Beacon,Packet Manager, andCommand Data Handlerrefactors—these are the core of the PR.Thank you for reviewing! Please reach out with any questions or suggestions.