Configurable thermal limit and pass minimum elevation #22
Conversation
Now that it's summer it's the perfect time to make the thermal limit configurable. During a demonstration a couple weeks ago we ran into the need for this where the in-box temperature was just slightly over the limit and we the operators were willing to just override it. This adds both a command line option and a config file option. The default value has also been raised from 30 to 40 on recommendation from Glenn.
Due to link budget issues lately we've been only wanting to consider passes above 45 degrees. This makes the value configurable via the config file.
bryborge
left a comment
bryborge
left a comment
There was a problem hiding this comment.
A couple of questions, comments, and a suggestion.
| * `alt` (Integer) - Station altitude in meters. | ||
| * `name` (String) - station name or callsign. | ||
| * `temperature-limit` (Float or Integer, optional) - Temperature in Celsius above which stops a | ||
| pass from being run to protect the hardware. Default: 40°C |
There was a problem hiding this comment.
Personally, I tend to prefer omitting real values like 40°C (and 15° above) from README docs. If we want to keep from having to update the documentation here if/when we change the config value for temperature-limit and minimum-pass-elevation, we could remove "Default: XX°C" with "see config for default settings," or something to that effect. That way there's a single source of truth, and the documentation does it's job by pointing people in the relevant direction for the up-to-date information.
There was a problem hiding this comment.
This is a good point, it'll be easy for the README to drift out of date. The hard part though is figuring out the best way to communicate default values to the user. There's a number of options I can think of:
- At the moment the source of truth is the class variables in
class Config. Pointing users directly to that isn't great since we shouldn't require that they can read Python. - Have the values output in the generated template config with expanded comments. But the template only fills out the non-optional values which don't or can't have defaults. Should we even have optional values? The TLE cache should be but everything else?
- Have a standalone template config somehow that we refer users to. This would make template generation more convenient.
- Have a command line flag that prints config info? Not particularly obvious.
- Does pydantic have a tool or recommended pattern for making this easier?
- Have a CI step that somehow reads the README and makes sure all the config options are documented and default values are correct. I've been wanting more automated checks of documentation and I think this would be the most interesting way of doing it but processing text could be a bit brittle.
There was a problem hiding this comment.
--show-defaults could do this well enough I think. Or write a small script that does string substitution for a section of the readme where we communicate all the default values to users. Something like:
<!-- DEFAULTS_START -->
render key/value table here, parsed from config.py
<!-- DEFAULTS_END -->
Then, in CI, you could have that script run as part of the release workflow (which perhaps we don't have a "formal" process for yet?).
There was a problem hiding this comment.
I looked a little more into it and pydantic does have a rich ecosystem of documentation generators (e.g. docdantic) so I think that's going to be the best way of doing this. It should be done when that gets implemented.
| slew: AzEl | None = None | ||
| beam_width: float | None = None | ||
| # FIXME: the limit should be retrieved from stationd instead of our toml but that feature | ||
| # doesn't exist. |
There was a problem hiding this comment.
Is this FIXME comment related to this issue in stationd? If stationd exposes it's cpu temperature, then pass-commander could totally still own this limit. Though I also see a strong argument for this piece of data to be owned by whatever code is closer to the ground station hardware.
There was a problem hiding this comment.
I was thinking more this issue, but that one is related as well. But yeah, my thinking was that stationd should know all the details and limits of operating a station safely. Pass commander shouldn't care about specific station values, only if it's OK to operate or not.
In talking with Glenn it turns out he expects the PAs to noticeably heat up the enclosure so we should make sure during the entire operation that we're within the expected thermal limits. This checks every 30 seconds which I assume is fine but maybe we can adjust this in practice.
As demonstrated a couple weeks ago and identified in the previous review, the limit for temperature at which pass-commander refuses to start a pass should be configurable, either for setting general policy from the config or quick operator override from the command line. While writing this I realized that stationd should probably be the one to know about the thermal limits of the station it's operating on and inform pass commander of it but that feature isn't implemented yet there so this is a good interim.
While I was there I also made minimum pass elevation a config option, while ideally we'd like to consider down to 15 degrees, currently we're only interested in passes above 45 degrees.