Bug audit fixes for dev preview#65
Conversation
tadelv
left a comment
tadelv
left a comment
There was a problem hiding this comment.
some comments inline. I get the need to sanitize and guard for edge cases, but the work could be split in (at least) half if the packing/unpacking would be unified, it seems.
| #board_build.partitions = <min_spiffs.csv> | ||
| board_build.filesystem = littlefs | ||
| monitor_speed = 115200 | ||
| upload_port = COM3 |
There was a problem hiding this comment.
do not specify upload ports this way, some users might have steam hardware on that port, or even have linux port designation
There was a problem hiding this comment.
Reverted the platformio.ini
| ; growth; complements the WS_BROADCAST_HEAP_FLOOR gate in include/websocket.h. | ||
| -D WS_MAX_QUEUED_MESSAGES=8 | ||
| !python3 git_rev_macro.py | ||
| !python git_rev_macro.py |
There was a problem hiding this comment.
absolutely sure about this one? keep in mind most important is the github action can run
There was a problem hiding this comment.
Reverted the platformio.ini
| async _enableNotification() { | ||
| await this.readCharacteristic.startNotifications(); | ||
| this.readCharacteristic.addEventListener('characteristicvaluechanged', this.notification_handler.bind(this)); | ||
| this.readCharacteristic.addEventListener('characteristicvaluechanged', this.notificationHandler); |
There was a problem hiding this comment.
are the characteristics used in the hosted version of Weigh_Save?
I believe websocket should be used to get weight data
There was a problem hiding this comment.
Yes: when Weigh_Save is served from the scale over HTTP, live weight comes from /snapshot WebSocket. The BLE characteristics are only for direct browser-to-scale connection support.
const ws = new ReconnectingWebSocket(ws://${window.location.host}/snapshot);
...
if (jsondata.grams !== undefined) {
const weight = Number(jsondata.grams);
...
ui.updateWeightDisplay(currentWeight);
scale.processWeight(currentWeight);
}
| if (mask & WSP_TIMER_STOP) { stopWatch.stop(); } | ||
| if (mask & WSP_TIMER_ZERO) { stopWatch.reset(); } | ||
| if (mask & WSP_SET_SAMPLES) { | ||
| scale.setSamplesInUse(samplesInUse); |
There was a problem hiding this comment.
which task is this call coming from, the main or the websocket task? possible race?
just want to be sure.
There was a problem hiding this comment.
scale.setSamplesInUse() runs from the main loop via the pending queue, not directly from the WebSocket task
| String pass = jsonObj["pass"]; | ||
|
|
||
| saveCredentials(ssid, pass); | ||
| saveCredentialsForRestart(ssid, pass); |
There was a problem hiding this comment.
why not forward error from nvs save to api response?
| void sendUsbGyro(); | ||
| #endif | ||
|
|
||
| static inline uint16_t encodeScaledUInt16(float value, float scale) { |
There was a problem hiding this comment.
we have encode in multiple places if i see this correctly. can we unify the command parsing as well as data serialization for ble and usb?
There was a problem hiding this comment.
Yes, there is some duplication in the encode helpers. That would need BLE/USB protocol-helper cleanup as a separate follow-up, with testing.
| return false; | ||
| } | ||
|
|
||
| uint8_t calculatePayloadChecksum(uint8_t *data, size_t len) { |
There was a problem hiding this comment.
same for checksum, my knowledge about the source might be poor, but we could simplify instead of have duplicates (if we actually have duplicates)
There was a problem hiding this comment.
Also for the checksum there are duplications. i keep that in mind
2 participants
Collects the current bug-audit fixes for a dev preview/tester build.
Scope:
BLE/USB protocol bounds,
WiFi setup/recovery,
OTA callback handling,
EEPROM persistence, and quick-boot status reporting.
WebSocket parsing guards,
CSP, preset parsing,
and invalid weight handling.