LH data in permanent storage#11
Conversation
ataffanel
left a comment
ataffanel
left a comment
There was a problem hiding this comment.
Looks good! A couple of error management that might need tightening.
| if !config.geos.is_empty() { | ||
| let geometries: HashMap<u8, LighthouseBsGeometry> = config | ||
| let progress_bar = make_progress(num_bs as usize, "Geometry", non_interactive); | ||
| for id in 0..num_bs { |
There was a problem hiding this comment.
This loop silently ignores any base-station IDs in the input YAML that are outside 0..num_bs. Before this change, those IDs went through write_geometries_with_progress()/write_geometry(), so the ID validation returned an error. Now a config containing e.g. geos: {16: ...} can report success while dropping user-provided data. Could we validate all config.geos/config.calibs keys before writing and fail if any ID is out of range?
| .unwrap_or_default(); | ||
| match lighthouse_mem.write_geometry(id, &geo).await { | ||
| Ok(()) => {} | ||
| Err(Error::MemoryError(_)) => { |
There was a problem hiding this comment.
Skipping every MemoryError is safe for padded default entries, but it also hides failures for base stations that were explicitly present in the YAML. For example, if the file contains BS 4 and the connected firmware only supports BS 0-3, this will skip BS 4, persist the rest, and still print success. Could we return an error when config.geos.contains_key(&id) (and similarly for config.calibs) and only ignore unsupported IDs for the padded invalidation entries?
There was a problem hiding this comment.
Good point. I have a fix in 12c858e that returns a hard error when an unsupported BS is found. However, the supported BSs in the yaml file will be written in RAM. Do you think that's ok?
2 participants
cfcli lh config writeonly wrote geometry and calibration data to RAM. It never sent the persist-data packet, so the uploaded lighthouse configuration was silently lost on the next reboot.Also, the write only touched the basestation IDs present in the input YAML file, leaving any previously-uploaded basestation data not in the file stale and still marked valid.