ldc1612: handle i2c errors

[nefelim4ag]

I feel somewhat obligated to finish i2c error handling, or at least push it a little forward after https://github.com/Klipper3d/klipper/pull/6689

I got some idea of how error handling happens around accelerometers, and can incorporate it into other i2c bulk data sensors.

So, here is a suggestion for LDC1612, the host code already handles cases where there are errors bit set in MSB. Then it should be easy to unconditionally set any of them, if i2c got an error, because ret is more than 0 on error, and less than 8.

    def _convert_samples(self, samples):
        ...
        for ptime, val in samples:
            mv = val & 0x0fffffff
            if mv != val:
                self.last_error_count += 1

If it would be better, I can manually hardcode enum success to 0, to make it more deterministic.

query_status handling is done in the same way as in the sensor_adxl345. Technically > 64 would be enough (1 << 6 == 64).

Not tested, because I just don't have ldc1612.

Thanks.

[ZeSlammy]

So ELI5, would this work on Temp, pressure, Gaz sensors plugged onto a PCB for a Stealth Max filter ? I somehow have NACK i2C errors and they are seen as critical even in Kalico/DangerKlipper

[nefelim4ag]

So ELI5

This PR will touch only inductive bed sensors, like BTT Eddy. So, not yet. I hope, there will be a series of patch sets with error handling in different places, where it is easy to do with the current code infrastructure. But that will not touch Temp, Pressure, or Gaz sensors.

To generally handle I2C errors, as discussed in the previous topic, there should be some command changes, and forwarding error information back to the host where they will be handled (possibly retried once). This can break something, this is a lot of work and will take several iterations.

Temp, pressure, Gaz sensors plugged onto a PCB for a Stealth Max filter ? I somehow have NACK i2C errors and they are seen as critical even in Kalico/DangerKlipper

I don't know anything about your setup and think you can try to ask for help on the discourse about what you can do. If you have separate MCUs for sensors and they do not control heaters. I can only suggest something like:

 void i2c_shutdown_on_err(int ret)
 {
+    return;
     switch (ret) {
     case I2C_BUS_NACK:
         shutdown("I2C NACK");

To disable I2C errors completely. This is bad, this is a hack and if you don't know how to do it, you should not.

Hope it helps.

[thijstriemstra]

I feel somewhat obligated to finish i2c error handling

Thanks! I've been following the progress ever since an bme280 i2c sensor stopped an important print due to an i2c error and disabled it ever since.

[github-actions[bot]]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards, ~ Your friendly GitIssueBot

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

[KevinOConnor]

Thanks for working on this. I'll try to test this on my ldc1612. Alas, I'm quite behind on projects so it may take me some time.

Cheers, -Kevin

[Supergun777]

Patch applied, everything works as before, don't know how to reproduce I2C errors

[nefelim4ag]

Just rebased, I did recheck the code if there is a appropriate error handling on in the LDC1612 code on the MCU and the Host side.

Seems it also should correctly report "Eddy current sensor error" on I2C communication error during the homing procedure.

    // High 4 bits are for errors
    d[0] |= ret << 4;

    // Check for endstop trigger
    uint32_t data =   ((uint32_t)d[0] << 24)
                    | ((uint32_t)d[1] << 16)
                    | ((uint32_t)d[2] << 8)
                    | ((uint32_t)d[3]);
    check_home(ld, data);
 ....
check_home(struct ldc1612 *ld, uint32_t data)
    if (data > 0x0fffffff) {
        // Sensor reports an issue - cancel homing
        ld->homing_flags = 0;

Thanks.

[nefelim4ag]

Taking into account that I now have the LDC, it would probably make sense to utilize error bits (we actually do intentionally enable them and report them to the host).

I think that for now, the handling of the status can be okay. For data, I'm not sure about a good solution. Probably it can be some special value like x0 or 0xdeadbeef. For now, I feel like setting sensor state bits inside data report with I2C errors may not be a great idea.

[nefelim4ag]

I spent some more time with sensors and all of the application notes. I guess we can utilize the under/over range bits: https://www.ti.com/lit/an/snoa959/snoa959.pdf

Under-range can't happen because we do not use offset, and if we do want it can be done in software to compact the bulk data output, and/or pack more status bits.

My (carto) board would output something about raw values in the range: 3_000_000...4_000_000. BTT Eddy, AFAIK, uses 12MHz, and has a similar frequency range. So the raw values would be twice as much. 6_000_000..8_000_000.

Overrange, on the other hand, is always 0xfffffff.

Watchdog/Zero count is something that should not happen, and honestly, both of them sound fatal to me.

[nefelim4ag]

Probably I've found an appropriate solution. Over/Under range are mutually exclusive, and even if they happen - they can be decoded by the value itself. So they can be used for error reporting. The Zero count is currently not reported anywhere, and I would not bother to report it specifically. It should be handled within watchdog error.

The only inconvenience is that there is low/high amplitude error aliasing. It is possible, though, to encode status and errors in 15 distinct values (4 bits). But I do not see that it is neccessary curently. If the driver current is calibrated (it should be), then there are predictable results of where amplitude errors happen.

Thanks.

[nefelim4ag]

Patch works.

LDC1612 starting 'carto' measurements
LDC1612: I2C IO error
LDC1612: I2C IO error

BTW, watchdog errors can be produced by shortening the coil.

Thanks.