Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for lis3dh accelerometer #6477

Closed
wants to merge 1 commit into from

Conversation

pk-love-tjy
Copy link

No description provided.

@Sineos
Copy link
Collaborator

Sineos commented Jan 29, 2024

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

MPU_DEV_IDS = {
0x74: "mpu-9515",
0x73: "mpu-9255",
0x71: "mpu-9250",
0x70: "mpu-6500",
0x68: "mpu-6050",
#everything above are normal MPU IDs
0x75: "mpu-unknown (DEFECTIVE! USE WITH CAUTION!)",
0x69: "mpu-unknown (DEFECTIVE! USE WITH CAUTION!)",
}
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
Copy link
Author

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
Copy link
Author

Signed-off-by: pengkua [email protected]

@Wulfsta
Copy link
Contributor

Wulfsta commented Jan 31, 2024

Signed-off-by: pengkua [email protected]

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 pk-love-tjy force-pushed the master branch 2 times, most recently from dfa0765 to c3b6fe6 Compare January 31, 2024 09:09
@pk-love-tjy
Copy link
Author

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
Copy link

SHKinsem commented Feb 1, 2024

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
Copy link
Contributor

archi commented Feb 1, 2024

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
Copy link
Author

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
Copy link
Author

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

lis2dw_query(struct lis2dw *ax, uint8_t oid)
{
uint8_t msg[7] = {0};
uint8_t fifo[2] = {LIS_FIFO_SAMPLES| LIS_AM_READ , 0};
uint8_t fifo_empty,fifo_ovrn = 0;
msg[0] = LIS_AR_DATAX0 | LIS_AM_READ ;
uint8_t *d = &ax->sb.data[ax->sb.data_count];
spidev_transfer(ax->spi, 1, sizeof(msg), msg);

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

@SHKinsem
Copy link

SHKinsem commented Feb 4, 2024

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
Copy link
Collaborator

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

@KevinOConnor KevinOConnor added the pending feedback Topic is pending feedback from submitter label Feb 16, 2024
Copy link

github-actions bot commented Mar 9, 2024

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.

@github-actions github-actions bot added the inactive Not currently being worked on label Mar 9, 2024
@github-actions github-actions bot closed this Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Not currently being worked on pending feedback Topic is pending feedback from submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants