Improve device driver discovery messaging#32859
Improve device driver discovery messaging#32859katzfey wants to merge 4 commits intoArduPilot:masterfrom
Conversation
|
Please do the QURT change in a separate PR. The thing with printf and DEV_PRINTF regardless is that they can corrupt mavlink messages. So this generally looks nice to collect them into a few places but what is done there may need to change. Otherwise we are just sending out more garbage! But maybe that's okay. |
|
I guess I don't really understand. We're talking about a very few messages at the very beginning of initialization that provide some very useful information. If this Mavlink corruption is such a big deal why allow printf, DEV_PRINTF at all? And some drivers already are using printf / DEV_PRINTF / hal.console->printf. Why has this been allowed if it's such a disaster? This PR looks like an improvement over what is there today. But I'll move the QURT stuff to another PR since it doesn't belong in here. |
|
It's not a major disaster but it's something to discuss. I've had to fix actual issues caused by this. |
katzfey
force-pushed
the
pr-printf-driver-discovery
branch
from
a108aef to
83d302c
Compare
|
QURT stuff moved out to PR #32861 |
peterbarker
left a comment
peterbarker
left a comment
There was a problem hiding this comment.
I really like the consistency this offers.
I've got similar patches floating around here which I've never put forward. You can roll a lot more info this common code (setting the bus ID and device type etc).
I'm uneasy with the printf. That's not a good idea on stm32. I suggest we create something along the lines of DEV_PRINTF but just for device discovery and use that. We should be able to make that just be printf on AP_HAL_QURT seeing that's what you're after.
| uint8_t register_sensor(void); | ||
| // slots it will panic. If name is non-null, prints a | ||
| // "<name> found on bus N id N address 0xN" discovery message. | ||
| uint8_t register_sensor(int32_t dev_id = 0, const char *name = nullptr); |
There was a problem hiding this comment.
It doesn't look like any caller is not passing in both values here - remove the defaulting?
Similarly elsewhere
There was a problem hiding this comment.
Yes, there are instances. For example, AP_Baro_MSP relies on defaults. AP_Baro_SITL, etc. Also for AP_Compass_Backend and AP_InertialSensor.
There was a problem hiding this comment.
Of course, those could all be changed to provide values also.
3 participants
Summary
Currently compass, baro, magnetometers, and GPS have a mish-mash of reporting device discovery. Some drivers do not report it, some report it with printf, some with hal.console->printf, and some with DEV_PRINTF. And the actual text varies as to what is reported. This PR attempts to standardize the reporting using printf with support in the underlying base class.
Classification & Testing (check all that apply and add your own)
Tested on VOXL3 with qmc5883l mag, dps310 barometer, bmi270 IMU, and ublox M10S GPS.