lis3dh, icm20948: Fix incorrect mux endpoint name, name typos

[MRX8024]

-Currently not possible to create a subscription to lis3dh via data logger. Data logger doesn't know about lis3dh's existence in the config, and the endpoint have a different name.

-Typos in logging/info names

-Double registration of default mux commands if two unnamed accelerometers are configured and adxl345 is missing.

[KevinOConnor]

Thanks. I'm not sure it's a good idea to introduce new endpoint names. How about the following instead?

--- a/scripts/motan/data_logger.py
+++ b/scripts/motan/data_logger.py
@@ -14,6 +14,7 @@ ConfigSubscriptions = [
     # (cfgtype, capture_name, api_request, request_params)
     ('adxl345', 'accelerometer:{csn}', '{ct}/dump_{ct}', {'sensor': '{csn}'}),
     ('lis2dw', 'accelerometer:{csn}', '{ct}/dump_{ct}', {'sensor': '{csn}'}),
+    ('lis3dh', 'accelerometer:{csn}', 'lis2dw/dump_lis2dw', {'sensor':'{csn}'}),
     ('mpu9250', 'accelerometer:{csn}', '{ct}/dump_{ct}', {'sensor': '{csn}'}),
     ('bmi160', 'accelerometer:{csn}', '{ct}/dump_{ct}', {'sensor': '{csn}'}),
     ('icm20948', 'accelerometer:{csn}', '{ct}/dump_{ct}', {'sensor': '{csn}'}),

-Kevin

[MRX8024]

Okay, that's a good solution too.

[KevinOConnor]

Thanks. Any chance you could update your signed-off-by line to include the angle brackets around the email address (ie, <maksim8024@gmail.com> instead of maksim8024@gmail.com)? It's not a "show stopper", but github doesn't allow me to easily correct that unless I use "squash and merge".

-Kevin

[MRX8024]

Okay, no problem, I'll do that. I need some more time, I accidentally found another bug with accelerometers, the fix for which I would like to attach to this PR.

[MRX8024]

Now everything should be fine; updated PR description.

[KevinOConnor]

Thanks. The first 3 commits look fine to me. I'm not sure about the 4th commit.

I'm not sure it's a good idea to register the commands based on order of initialization, as that can lead to confusing behavior. I think its fine to raise an error if there are two accelerometer config sections without an explicit "chip name" (as opposed to choosing one that becomes the default for commands). Also, fwiw, if you're going to have a global variable I think it would be preferable to just declare a global variable and avoid using Python class variables.

Cheers,
-Kevin

[MRX8024]

Thanks. I'm not sure it's a good idea to introduce new endpoint names. How about the following instead?

FYI, if used only one endpoint, and lis2dw and lis3dh have the same name (which is allowed), you seems should get the error "mux endpoint ... already registered" in webhooks. What do you think about this?

[KevinOConnor]

FYI, if used only one endpoint, and lis2dw and lis3dh have the same name (which is allowed), you seems should get the error "mux endpoint ... already registered" in webhooks. What do you think about this?

I'd say that is not a valid configuration and hopefully the user can figure it out from the error message. We could improve the error message, but for what it is worth, we could spend an eternity writing code that produces error messages and still not cover every case...

Separately, in regard to this PR, the first 3 patches look okay. The fourth patch looks a little "ugly" to me - I agree we can't use a global variable due to restart handling, but registering a disconnect handler seems overkill. How about we commit the first three patches and handle this issue separately?

If you do want to improve the error message on redundant g-code registration, then consider adding a try: / except gcode.error: block in register_commands().

Thanks again,
-Kevin

[MRX8024]

Alright, I agree that this isn’t so easy to solve “cleanly,” and it will likely require refactoring the accelerometers in the project.

If you still don't like the updated latest commits, I'll revert them and we'll think about how to fix this differently in the future.

If the changes are satisfactory, should we add a note about this in config_changes?

[KevinOConnor]

The changes look okay to me. If there are no other comments I'll look to commit in a day or so.

If the changes are satisfactory, should we add a note about this in config_changes?

I'd say it doesn't seem necessary, but I don't mind if you'd like to add a note.

Thanks again,
-Kevin

[KevinOConnor]

Thanks.

-Kevin