search

Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.99k stars 5.17k forks source link

Simplification of host bulk sensor code #6432

Closed KevinOConnor closed 6 months ago

KevinOConnor commented 6 months ago

This PR introduces new klippy/extras/bulk_sensor.py code and is intended to simplify the existing adxl345.py, mpu9250.py, lis2dw.py, and angle.py code. This PR is an internal rework and should not produce a user noticeable change.

The existing host "bulk sensor" modules have a lot of duplicate code. This makes it harder to maintain these modules, and makes it harder to add new "bulk sensors". The goal of this PR is to move much of the low-level message handling code to bulk_sensor.py and to keep the per-chip modules focused just on chip specific logic. Ideally this will make it easier to add future "bulk sensors".

This PR will also facilitate refactoring of the mcu code. MCU code changes will require users to reflash their MCUs, so I'll plan on introducing those changes in a separate PR.

@garethky - FYI.

-Kevin

garethky commented 6 months ago

Exciting! Looking forward to what the MCU code looks like.

Coincidentally, I re-based last night, and got the dreaded "Too many message ids" error when compiling the MCU code. Maybe this work will hold off having to deal with that. (demo branch: https://github.com/garethky/klipper/tree/adc-endstop-rebase)

KevinOConnor commented 6 months ago

I have published the corresponding MCU cleanups to a dev branch at: https://github.com/KevinOConnor/klipper-dev/tree/work-bulksensor-20231217

That dev branch also includes changes from this PR and PR #6433.

Separately, I think it would be useful if we could support the hx71x and ads1263 as "bulk sensors" in the mainline Klipper repo today. I understand that there is ongoing work with load cells, but I think the basic "bulk sensor" support (as exportable over webhooks/motan) has value on its own.

-Kevin

garethky commented 6 months ago

Sounds good. I'm happy to refactor what I have to work with sensor_bulk.

I'll need a way to call the adc_endstop with each new sample [ref]. Taking sensor_bulk as a pattern, I'd be doing something inside the sensor_x code. Is that the way you think it should work?

I don't personally have a strong reason to submit the ADS1263 code, I was planning on deleting it for an eventual PR. Its very much a prototyping thing and I don't think there will be any commercial interest in having it in klipper. But if you see the value for hobbyists and tinkering then I can keep it alive.

KevinOConnor commented 6 months ago

If I understand your code correctly, you have a "multiplex_adc" which has a pointer to an "hx71x" and has a pointer to a "loadcell_endstop". The "loadcell_endstop" has a pointer to a "trsync".

For what it is worth, I suspect "multiplex_adc" will not scale well. It'll be difficult to get different sensor chips to conform to a single interface.

An alternate approach would be an "hx71x" that can read its own measurements, populate its own "sensor_bulk" reports, and communicate them directly to a host "hx71x.py" module. That would allow the most flexibility in implementing future chip quirks. In this alternate scheme, I'd imagine that "hx71x" would have a pointer to an "adc_endstop" (which in turn would have a pointer to a trsync and would determine when to trigger it). For a short-term merge, the "adc_endstop" could be omitted.

It's difficult to make judgements on code structure, however. I'm not entirely certain on all the requirements of load cells.

If you don't think the ADS1263 has immediate value, then I wouldn't spend time on it.

Cheers, -Kevin

garethky commented 6 months ago

Ok, I can go down that path, structurally, and we can see how it looks