Klipper3d / klipper

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

support for lis3dh accelerometer #6477

Closed pk-love-tjy closed 3 months ago

Sineos commented 4 months ago

As far as I can tell, the LIS2DH and LIS3DH are register compatible. There should be no need to create extra driver modules for it. If this is correct, refer to https://github.com/Klipper3d/klipper/blob/9f41f53c5e364694b9b41279b3b3aee34250b93a/klippy/extras/mpu9250.py#L12-L21 how to handle multiple compatible device IDs.

Please also refer to point 3 in "What to expect in a review" in master/docs/CONTRIBUTING.md and provide a signed off by line.

pk-love-tjy commented 4 months ago

Lis3dh and lis2dw only have the same register address, but their meanings are different. image Of course, the following forms can also be used: image But this may affect the aesthetics of the code.

pk-love-tjy commented 4 months ago

Signed-off-by: pengkua pengkua.tjy@gmail.com

Wulfsta commented 4 months ago

Signed-off-by: pengkua pengkua.tjy@gmail.com

Squash the commits into one and add a sign off there; make sure you follow the contributing guide for the format.

Do you need testers? If this is stable I can try it on a Duet3D 1LC.

pk-love-tjy commented 4 months ago

Thank you for your reminder. I have already turned commit Squash into one and add sign off. I tested using lis3dh and the results were similar to using adxl345. You can try it on Duet3D 1LC.

SHKinsem commented 4 months ago

I am testing your code. After some simple setup, I can use "ACCELEROMETER_QUERY chip=lis3dh" to query the data. However, I am having trouble with "SHAPER_CALIBRATE" and "TEST_RESONANCES AXIS=X", which would turn out "!! Invalid adxl345 id (got 0 vs e5)."

I also tried specifying the chip with suffix "CHIPS=lis3dh" or 'CHIPS="lis3dh" ' and so on, none of them worked. Did I miss something? Thx.

archi commented 4 months ago

I've aleady pointed this out on the Discord, but I think I'll replicate it here:

  1. (boring part) The register map for LIS3DH and LIS2DW are the same, but the meanings of the bits are indeed different. If I were to decide, I would still merge it in one file since the remainder of the logic seems to be too similar; and this avoids code duplication. But that's just my opinion and I still 👏 everyone contributing to FOSS :)
  2. (btw) There is also a LIS2DH part, that one seems to fully register-compatible with the LIS3DH, except for the aux ADC:
  3. (interesting part) The lis3dh has an aux ADC with three input channels. How about exposing them? Might be nice for a follow-up PR since I understand this is a lot to ask.

My idea is to permanently put the LIS2SH on the delta's effector, and then try using the ADC for a hall effect filament width sensor (or at least a filament presence sensor). It's cheaply (~40c) available on jlcpcb, making them interesting for anyone looking to fabricate a hobby PCB (Caveat emptor: The aux ADC should survive Vcc, but its input range is actually 0.8-1.6V).

pk-love-tjy commented 4 months ago

I am testing your code. After some simple setup, I can use "ACCELEROMETER_QUERY chip=lis3dh" to query the data. However, I am having trouble with "SHAPER_CALIBRATE" and "TEST_RESONANCES AXIS=X", which would turn out "!! Invalid adxl345 id (got 0 vs e5)."

I also tried specifying the chip with suffix "CHIPS=lis3dh" or 'CHIPS="lis3dh" ' and so on, none of them worked. Did I miss something? Thx.

Thank you for your help in testing. I cannot reproduce the problem you mentioned. I pulled the code from GitHub again, compiled and burned it, but I am unable to reproduce this issue. My partner also works reliably using it. image

You can try using my configuration again. [resonance_tester] accel_chip: lis3dh probe_points: 60, 60, 20 [lis3dh] axes_map: y,z,x cs_pin: SHT36:gpio9 spi_software_sclk_pin: SHT36:gpio10 spi_software_mosi_pin: SHT36:gpio11 spi_software_miso_pin: SHT36:gpio12

pk-love-tjy commented 4 months ago

I've aleady pointed this out on the Discord, but I think I'll replicate it here:

  1. (boring part) The register map for LIS3DH and LIS2DW are the same, but the meanings of the bits are indeed different. If I were to decide, I would still merge it in one file since the remainder of the logic seems to be too similar; and this avoids code duplication. But that's just my opinion and I still 👏 everyone contributing to FOSS :)
  2. (btw) There is also a LIS2DH part, that one seems to fully register-compatible with the LIS3DH, except for the aux ADC:
  3. (interesting part) The lis3dh has an aux ADC with three input channels. How about exposing them? Might be nice for a follow-up PR since I understand this is a lot to ask.

My idea is to permanently put the LIS2SH on the delta's effector, and then try using the ADC for a hall effect filament width sensor (or at least a filament presence sensor). It's cheaply (~40c) available on jlcpcb, making them interesting for anyone looking to fabricate a hobby PCB (Caveat emptor: The aux ADC should survive Vcc, but its input range is actually 0.8-1.6V).

@archi Thank you for your suggestion. It would indeed be better to merge them.

I was studying Klipper, and my intention was to not modify the code to add features. I was afraid that my changes would cause unpredictable errors. What should I do if I want to merge them? Should I close this merge first, and then reopen it after the modifications are completed?

[ May I ask a question ] lis3dh_query the function, it seems that lis3dh cannot read continuously from 0x28 like lis2dw. I couldn't find the relevant part from the datasheet, maybe I overlooked it.

https://github.com/pk-love-tjy/klipper/blob/d2dbf1d37e4921476f920bd535dd6f957702d530/src/sensor_lis3dh.c#L69-L78 https://github.com/Klipper3d/klipper/blob/9f41f53c5e364694b9b41279b3b3aee34250b93a/src/sensor_lis2dw.c#L69-L78

The ADC above can indeed do some interesting things, but I don't think ADC will be missing in Toolhead's MCU.

SHKinsem commented 4 months ago

I am testing your code. After some simple setup, I can use "ACCELEROMETER_QUERY chip=lis3dh" to query the data. However, I am having trouble with "SHAPER_CALIBRATE" and "TEST_RESONANCES AXIS=X", which would turn out "!! Invalid adxl345 id (got 0 vs e5)." I also tried specifying the chip with suffix "CHIPS=lis3dh" or 'CHIPS="lis3dh" ' and so on, none of them worked. Did I miss something? Thx.

Thank you for your help in testing. I cannot reproduce the problem you mentioned. I pulled the code from GitHub again, compiled and burned it, but I am unable to reproduce this issue. My partner also works reliably using it. You can try using my configuration again.

It is now working flawlessly now! It turned out that I stupidly forgot to comment out my old config of adxl. However, I dont know why use the coommand with the chip specified did not work (Could also be my misuse of command lol)

Anyways, thank you for your contribute! It makes my SHT36 canboard more worthy now XD

KevinOConnor commented 4 months ago

Thanks. I think it would be useful to add support for lis3dh. Some comments:

  1. As others have mentioned, I think it would be useful to merge this into the existing support. In particular, I'd be curious if sensor_lis2dw.c could be used unchanged. The only thing that cares about the register bits is lis2dw.py:_start_measurements(), so I'd just create _start_measurements_lis2dw() and _start_measurements_lis3dh() in the python code.
  2. The signed-off-by line needs to contain your full real name (it's not clear to me if pengkua is your full real name). https://www.klipper3d.org/CONTRIBUTING.html#format-of-commit-messages

Should I close this merge first, and then reopen it after the modifications are completed?

You can either update this PR or create a new one if you prefer.

lis3dh_query the function, it seems that lis3dh cannot read continuously from 0x28 like lis2dw. I couldn't find the relevant part from the datasheet, maybe I overlooked it.

Many SPI and I2C devices support multiple reads per transaction. Have you tested that it does not work on the lis3dw?

-Kevin

github-actions[bot] commented 3 months ago

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.