REST API: allow for voltage change on board#30
Conversation
4ed8663 to
596e83b
Compare
|
Rebased onto current main. |
rkuester
left a comment
There was a problem hiding this comment.
Thanks for working on this, and for your patience waiting for my review. This is a great first controllable parameter to push through the API. If we get the patterns right here, the next ones (fan speed, frequency, etc.) should fall into place much more easily. With that in mind, I have some feedback on the approach.
-
Follow the target/actual pattern I used for fans. Instead of patching
voltage_vdirectly, add atarget_voltage_vfield toPowerDomain(see rename below) alongside the existingvoltage_v(which stays as the measured reading). Separate fields because targets are optional (null means no override; the miner controls the value automatically) and because hardware may not achieve exactly what you request (target_voltage_vof 1.1 might read back asvoltage_vof 1.098). See the "Target fields" section indocs/api.mdfor how this convention works.This also means
PowerPatchisn't needed. The PATCH body should use the same inner types as the GET response (Fan,PowerDomain, etc.) with the writabletarget_*fields. Thenamefield in each array element identifies which fan or power domain to update; read-only fields are ignored.BoardPatchRequestis still needed as a thin wrapper with optional top-level arrays, but the inner types are shared. Adding new controllable fields later just means adding atarget_*field to the existing type.For example,
GET /api/v0/boards/bitaxe-71bfd369returns the full board telemetry:{ "name": "bitaxe-71bfd369", "model": "Bitaxe Gamma", "fans": [ { "name": "fan", "rpm": 4200, "percent": 100, "target_percent": null } ], "powers": [ { "name": "core", "voltage_v": 1.148, "voltage_min_v": 1.0, "voltage_max_v": 1.25, "target_voltage_v": null, "current_a": 8.2, "power_w": 9.3 } ] }PATCH /api/v0/boards/bitaxe-71bfd369accepts a partial update using the same inner types. To set the core voltage:{ "powers": [ { "name": "core", "target_voltage_v": 1.1 } ] }The
powersarray here deserializes into the samePowerDomaintype that GET serializes. Read-only fields likevoltage_vare simply ignored if present. TheBoardPatchRequestwrapper just makes the top-level arrays optional so the client only sends the parts it wants to change.While we're at it, rename
PowerMeasurementtoPowerDomain. Once it carries limits and a settable target alongside the readings, it's not just a measurement anymore. This is consistent withFan, which already has both measured and settable fields and isn't calledFanMeasurement. Good candidate for a preparatory commit. -
Enforce ASIC-safe voltage limits, not just regulator limits. The TPS546 config allows 1.0-2.0V, but the BM1370 can't safely take anywhere near 2.0V. esp-miner limits the BM1370 to 1.0-1.25V and we should match that. Reject out-of-range values with an error, don't clamp. Since the allowed range is board-specific and can't go in the OpenAPI spec, expose it per power domain in the board telemetry (e.g.
voltage_min_v/voltage_max_vonPowerDomain) so API clients can discover it at runtime. -
Return error details from
patch_board. Right nowOk(Ok(Err(_)))maps to a bare 422 with no body, so the caller can't tell whether they sent an unknown domain or an out-of-range voltage. Include the error message in the response. -
Make
cmd_txnon-optional inBoardRegistration. Instead of boards like the CPU miner setting it toNone, every board should create a command channel and return errors from the handler for unsupported commands. This avoids burdening every caller withOptionhandling when nearly all boards will have command channels. -
Use a type alias for the regulator type.
Arc<Mutex<Tps546<BitaxeRawI2c>>>is spelled out in multiple places. Atype Regulator = ...alias would clean all of them up (good candidate for a preparatory commit). Also,handle_board_commandspells out full crate paths instead of using the existing imports. -
Split the PR into atomic commits. It's currently a single commit covering types, API plumbing, board wiring, and the endpoint handler all at once. Each logical change should be its own commit so it's reviewable in isolation and bisectable if something breaks. For example, something like:
refactor(bitaxe): add Regulator type alias feat(board): add command channel to BoardRegistration feat(api): add find_board helper to BoardRegistry feat(api): add target_voltage_v to PowerMeasurement feat(bitaxe): handle SetVoltage command in stats monitor feat(api): add PATCH /boards/{name} endpoint docs(api): document board PATCH endpointSee
CONTRIBUTING.mdfor commit message format and atomicity guidelines. -
Fold
docs/voltage_change_api.mdintodocs/api.md. The endpoint reference belongs alongside the other endpoints, and the implementation walkthrough will go stale as code changes.
A write-up of this implementation can be found here.