Conversation
vjardin
commented
Nov 11, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR adds 11 new PMBus commands to the tool, significantly expanding its functionality for device configuration and inspection. The commands cover capability checks, interleaving, HRR (Hybrid Regulated Ratio) options, voltage thresholds, frequency control, alerts, and write protection.
- Adds comprehensive command implementations with get/set operations and JSON output
- Implements detailed help documentation for complex commands (hrr, capability)
- Extends PMBus opcode definitions to support new register addresses
- Provides both raw and decoded value access for flexibility
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/capability_cmd.c | Implements CAPABILITY register decode with optional requirement checking |
| src/interleave_cmd.c | Handles phase count and index configuration |
| src/mfr_hrr.c | Controls MFR_SPECIAL_OPTIONS with detailed help and bit manipulation |
| src/vin_cmd.c | Manages VIN_ON/OFF thresholds with LIN16U encoding support |
| src/pgood_cmd.c | Configures POWER_GOOD_ON/OFF window thresholds |
| src/freq_cmd.c | Controls frequency switch register |
| src/salert_cmd.c | Manages SMBALERT_MASK configuration |
| src/mfr_addr_offset.c | Handles MFR_OFFSET_ADDRESS manipulation |
| src/mfr_ramp_data.c | Reads vendor ramp data as hex blob |
| src/mfr_status_data.c | Retrieves vendor status snapshot |
| src/write_protect_cmd.c | Controls write protection levels |
| src/pmbus_io.h | Adds new PMBus opcode definitions |
| src/main.c | Integrates all new commands into main dispatcher |
| src/meson.build | Adds new source files to build |
| README.md | Documents all 11 commands with examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7bdf43c to
442669a
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/main.c:118
- [nitpick] Extra blank line added before usage() call. This appears to be an unintentional formatting change.
usage(argv[0]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1d969d8 to
deb870a
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
93a17e0 to
a2844ed
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2a4d7ef to
6ab8b55
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6ab8b55 to
7bc9634
Compare
7bc9634 to
97aa879
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
97aa879 to
c7cfb91
Compare
c7cfb91 to
4aa861a
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| put_temp_limit(json_t *dst, const char *key, int fd, uint8_t cmd) { | ||
| int w = pmbus_rd_word(fd, cmd); | ||
| json_t *o = json_object(); | ||
|
|
||
| if (w >= 0) { | ||
| uint16_t raw = (uint16_t) w; | ||
| double C = lin11_to_double(raw); | ||
| int8_t E = (int8_t) sign_extend((raw >> 11) & 0x1F, 5); | ||
| int16_t Y = (int16_t) sign_extend(raw & 0x7FF, 11); | ||
|
|
||
| json_object_set_new(o, "raw", json_integer(raw)); | ||
| json_object_set_new(o, "C", json_real(C)); | ||
| json_object_set_new(o, "lin11_exp", json_integer(E)); | ||
| json_object_set_new(o, "lin11_man", json_integer(Y)); | ||
| } else { | ||
| json_object_set_new(o, "error", json_integer(w)); | ||
| } | ||
|
|
||
| json_object_set_new(dst, key, o); | ||
| } | ||
|
|
||
| static void | ||
| put_temp_read(json_t *dst, const char *key, int fd, uint8_t cmd) { | ||
| int w = pmbus_rd_word(fd, cmd); | ||
| json_t *o = json_object(); | ||
|
|
||
| if (w >= 0) { | ||
| uint16_t raw = (uint16_t) w; | ||
| double C = lin11_to_double(raw); | ||
| int8_t E = (int8_t) sign_extend((raw >> 11) & 0x1F, 5); | ||
| int16_t Y = (int16_t) sign_extend(raw & 0x7FF, 11); | ||
|
|
||
| json_object_set_new(o, "raw", json_integer(raw)); | ||
| json_object_set_new(o, "C", json_real(C)); | ||
| json_object_set_new(o, "lin11_exp", json_integer(E)); | ||
| json_object_set_new(o, "lin11_man", json_integer(Y)); | ||
| } else { | ||
| json_object_set_new(o, "error", json_integer(w)); | ||
| } | ||
|
|
||
| json_object_set_new(dst, key, o); | ||
| } |
There was a problem hiding this comment.
[nitpick] Code duplication: The functions put_temp_limit (lines 144-163) and put_temp_read (lines 166-185) are nearly identical except for the JSON key names and function names. Consider extracting a common helper function to reduce duplication and improve maintainability.
| hex[n * 2] = '\0'; | ||
| json_object_set_new(o, "hex", json_string(hex)); | ||
| free(hex); | ||
| json_add_hex_ascii(o, "hex", b, (size_t)n); |
There was a problem hiding this comment.
Potential buffer overflow: json_add_hex_ascii is called without checking the return value. If n is large enough (SIZE_MAX/2 or greater), the function will fail and return -1, but the error is silently ignored. Consider checking the return value and handling the error appropriately.
| json_add_hex_ascii(o, "hex", b, (size_t)n); | |
| int hex_ret = json_add_hex_ascii(o, "hex", b, (size_t)n); | |
| if (hex_ret < 0) { | |
| fprintf(stderr, "json_add_hex_ascii failed\n"); | |
| json_decref(o); | |
| return 1; | |
| } |
| if (bestAbsY < 0) { | ||
| /* pick E to bring into range */ | ||
| int E = 0; | ||
| long Y = 0; | ||
| for (E = 15; E >= -16; --E) { | ||
| double scaled = (E >= 0) ? (v / (double) (1u << E)) : (v * (double) (1u << (-E))); | ||
| Y = lround_compat(scaled); | ||
| if (Y >= -1024 && Y <= 1023) | ||
| break; | ||
| } | ||
| if (Y < -1024) | ||
| Y = -1024; | ||
| if (Y > 1023) | ||
| Y = 1023; | ||
| bestE = E; | ||
| bestY = Y; | ||
| } |
There was a problem hiding this comment.
[nitpick] Potential infinite loop: In the conversion loop from double to LIN11 format, if v is exactly zero, the function returns early at line 50-51. However, if v is very close to zero but not exactly zero, the loop at lines 58-71 may not find a valid representation. The condition at line 74 (if (bestAbsY < 0)) handles this, but the subsequent fallback loop (lines 78-83) could theoretically fail to converge for edge cases. Consider adding an explicit check or comment about the convergence guarantee.
| exp5 = atoi(argv[++i]); | ||
| have_exp = 1; | ||
| } | ||
| } | ||
| if (!have_exp) { | ||
| /* Auto-discover exp5 from VOUT_MODE if not provided */ | ||
| if (pmbus_get_vout_mode_exp(fd, &exp5) == 0) | ||
| have_exp = 1; | ||
| } | ||
| int won = pmbus_rd_word(fd, PMBUS_POWER_GOOD_ON); | ||
| int wof = pmbus_rd_word(fd, PMBUS_POWER_GOOD_OFF); | ||
| if (won < 0 || wof < 0) { | ||
| perror("PGOOD_*"); | ||
| return 1; | ||
| } | ||
|
|
||
| json_t *o = json_object(); | ||
| json_object_set_new(o, "PGOOD_ON_raw", json_integer((uint16_t) won)); | ||
| json_object_set_new(o, "PGOOD_OFF_raw", json_integer((uint16_t) wof)); | ||
| if (!raw && have_exp) { | ||
| json_object_set_new(o, "PGOOD_ON_V", json_real(pmbus_lin16u_to_double((uint16_t) won, exp5))); | ||
| json_object_set_new(o, "PGOOD_OFF_V", json_real(pmbus_lin16u_to_double((uint16_t) wof, exp5))); | ||
| json_object_set_new(o, "exp5", json_integer(exp5)); | ||
| } | ||
|
|
||
| json_print_or_pretty(o, pretty); | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| if (!strcmp(argv[0], "set")) { | ||
| const char *on_raw = NULL, *off_raw = NULL, *on_v = NULL, *off_v = NULL; | ||
| int have_exp = 0, exp5 = 0; | ||
| for (int i = 1; i < argc; i++) { | ||
| const char *a = argv[i]; | ||
| if (!strcmp(a, "--on-raw") && i + 1 < argc) | ||
| on_raw = argv[++i]; | ||
| else if (!strcmp(a, "--off-raw") && i + 1 < argc) | ||
| off_raw = argv[++i]; | ||
| else if (!strcmp(a, "--on") && i + 1 < argc) | ||
| on_v = argv[++i]; | ||
| else if (!strcmp(a, "--off") && i + 1 < argc) | ||
| off_v = argv[++i]; | ||
| else if (!strcmp(a, "--exp5") && i + 1 < argc) { | ||
| exp5 = atoi(argv[++i]); | ||
| have_exp = 1; |
There was a problem hiding this comment.
Missing validation: The function atoi is used to parse the --exp5 argument without error checking. If the user provides non-numeric input, atoi returns 0, which is a valid exponent value and thus indistinguishable from an error. Consider using strtol with proper error checking for consistency with other commands.
4aa861a to
efba65c
Compare
Add a substantial set of PMBus feature commands, aligned with Flex BMR
(BMR685/BMR456/BMR480) docs.
Include some long helps and some notes (see TODO).
1) capability
- capability get
- capability check --need-pec on|off --min-speed 100|400 --need-alert on|off [--strict]
Decodes: PEC support, max bus speed (100/400 kHz), SMBALERT# presence,
and reserved low bits.
2) interleave
- interleave get
- interleave set [--set 0xNN] [--phases 1..16 --index 0..15]`
Upper nibble = phases-1, lower nibble = phase index.
3) hrr
- hrr get
- hrr set [--pec on|off] [--hrr on|off] [--dls linear|nonlinear] [--artdlc on|off] [--dbv on|off]`
- hrr set --raw 0xNN
Bits: bit7=PEC require, bit6=HRR, bit5=DLS slope, bit3=ART/DLC, bit2=DBV.
Includes hrr help with detailed descriptions and examples.
TODO: apply the same for other commands.
4) vin
- vin get [--exp5 N] [--raw]
- vin set [--on V] [--off V] [--exp5 N] | [--on-raw 0xNNNN] [--off-raw 0xNNNN]
5) pgood
- pgood get [--exp5 N] [--raw]
- pgood set [--on V] [--off V] [--exp5 N] | [--on-raw 0xNNNN] [--off-raw 0xNNNN]
6) freq
- freq get
- freq set --raw 0xNNNN
7) salert
- salert get
- salert set --raw 0xNN
8) addr-offset
- addr-offset get
- addr-offset set --raw 0xNN
9) ramp-data
Returns a hex blob (TODO: no vendor decode).
10) status-data
Returns a hex blob (concise status history snapshot).
11) write-protect
- write-protect get
- write-protect set [--none|--ctrl|--nvm|--all] | --raw 0xNN
Typical values: 0x00, 0x40, 0x80, 0xFF.
- No behavior changes for the existing commands.
- PEC: Enabling via hrr will require the host stack to send SMBus PEC or
communication will fail; see comments from hrr help.
The scaling logic can be incorrect. When exp5 >= 0, the condition (1 << (-exp5)) attempts a left shift with a negative value, which is undefined behavior. The logic should be inverted: when exp5 is negative, use (1 << (-exp5)), and when positive, use division. The simplest option is to use math.h's ldexp(). It seems it does not require libm.
Since it is not an obvious value to be set and it'll be very likely available from most devices from VOUT_MODE, let's read first the needed value instead of using the arguments. Let's keep the --exp5 argument if we to do not trust the read values.
Factorize the code to util_json.c
Centralize the lin16 code. It should be used to better apply the specifications based on PMBus-Specification-Rev-1-3-1-Part-II-20150313.pdf from https://pmbus.org/specification-archives/
Some BMR do not support an automatic restart. The workaround can be to generate a fault that leads to a shutdown until the fault is cleared. For instance, we can set the fault on the temperature.
Based on the PMBus specifiations, add few cases that were not clearly described by the Flex/BMR documentation.
efba65c to
55a9813
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (x >= 65535.0) | ||
| return 65535; | ||
|
|
||
| return (uint16_t)(x + 0.5); |
There was a problem hiding this comment.
The saturation logic is incomplete. If x is negative or NaN, it returns 0 at line 14. However, if x is between 0 and 65535 but negative after checking line 16, the cast at line 19 will produce incorrect results. For values slightly below 0 (e.g., -0.3), x + 0.5 becomes a small positive or negative value, and casting to uint16_t will wrap around. Add explicit saturation: if (x < 0.0) return 0; before line 19.
| double v = strtod(on_v, NULL); | ||
| won = units_to_lin16u(v, exp5); |
There was a problem hiding this comment.
Missing error handling for strtod. Invalid input in on_v will silently convert to 0.0. Add validation using an end pointer and check errno, consistent with the pattern used for parse_u16 calls in this same file.
| double v = strtod(off_v, NULL); | ||
| wof = units_to_lin16u(v, exp5); |
There was a problem hiding this comment.
Missing error handling for strtod. Malformed input in off_v will be silently converted to 0.0 without validation. Add proper error checking with an end pointer to detect conversion failures.
| for (int E = -16; E <= 15; ++E) { | ||
| double scaled = (E >= 0) ? (v / (double) (1u << E)) : (v * (double) (1u << (-E))); | ||
| long Y = lround_compat(scaled); | ||
| if (Y < -1024 || Y > 1023) | ||
| continue; | ||
| long a = (Y >= 0) ? Y : -Y; | ||
| if (a > bestAbsY) { | ||
| bestAbsY = a; | ||
| bestE = E; | ||
| bestY = Y; | ||
| if (bestAbsY == 1023) | ||
| break; |
There was a problem hiding this comment.
The inner loop performs 32 iterations for each value conversion. For E >= 0, using 1u << E directly is fine, but for negative exponents the division/multiplication pattern could be optimized. Consider using ldexp(v, -E) which is typically optimized by the compiler and avoids explicit branching. This would simplify to double scaled = ldexp(v, -E);
| for (E = 15; E >= -16; --E) { | ||
| double scaled = (E >= 0) ? (v / (double) (1u << E)) : (v * (double) (1u << (-E))); | ||
| Y = lround_compat(scaled); | ||
| if (Y >= -1024 && Y <= 1023) | ||
| break; |
There was a problem hiding this comment.
Similar to the optimization suggestion above, this loop could benefit from using ldexp(v, -E) instead of conditional branching with explicit shifts and division/multiplication.
| " temp read [all|t1|t2|t3]\n" | ||
| " temp help\n" | ||
| "\n" | ||
| "Notes: values are Celsius by default; suffix K or F is accepted (e.g., 358K, 185F, have fun).\n" |
There was a problem hiding this comment.
[nitpick] The comment 'have fun' is informal and could be perceived as unprofessional in a production tool's help text. Consider removing it or replacing with a more neutral phrasing like 'for example' or simply ending at '185F'.
| "Notes: values are Celsius by default; suffix K or F is accepted (e.g., 358K, 185F, have fun).\n" | |
| "Notes: values are Celsius by default; suffix K or F is accepted (e.g., 358K, 185F).\n" |
| int req = atoi(min_speed); /* expect 100 or 400 or 1000 */ | ||
| unsigned code = (cap >> 5) & 0x3u; | ||
| int khz = -1; | ||
| (void) speed_text(code, &khz); | ||
| int ok = (khz >= req); /* reserved -> khz==-1 -> fail */ | ||
| add_check(checks, "bus_speed_ok", ok, mism); |
There was a problem hiding this comment.
Using atoi without error checking. If min_speed contains non-numeric input or values that overflow, atoi returns 0 or undefined results without signaling an error. Use strtol with proper error handling, consistent with other parsing in this codebase (e.g., lines 52, 67, 157 in other files).
| int req = atoi(min_speed); /* expect 100 or 400 or 1000 */ | |
| unsigned code = (cap >> 5) & 0x3u; | |
| int khz = -1; | |
| (void) speed_text(code, &khz); | |
| int ok = (khz >= req); /* reserved -> khz==-1 -> fail */ | |
| add_check(checks, "bus_speed_ok", ok, mism); | |
| char *endptr = NULL; | |
| errno = 0; | |
| long req_long = strtol(min_speed, &endptr, 10); /* expect 100 or 400 or 1000 */ | |
| if (endptr == min_speed || *endptr != '\0' || errno == ERANGE || req_long < 0 || req_long > INT_MAX) { | |
| fprintf(stderr, "Invalid value for --min-speed: '%s'\n", min_speed); | |
| add_check(checks, "bus_speed_ok", 0, mism); | |
| } else { | |
| int req = (int)req_long; | |
| unsigned code = (cap >> 5) & 0x3u; | |
| int khz = -1; | |
| (void) speed_text(code, &khz); | |
| int ok = (khz >= req); /* reserved -> khz==-1 -> fail */ | |
| add_check(checks, "bus_speed_ok", ok, mism); | |
| } |