FWIW, I would first verify the hx71x to temperature is something that needs to be solved. If I understand the recent comments, it’s not clear the printers are actually used this way. Even if they are, it’s not immediately clear there would be many Klipper users looking to use that hardware with Klipper.
If a solution was needed, one possibility is to look at how klippy/extras/spi_temperature.py handles values from the MAXxxxxx thermocouple/rtd chips and converts them to a temperature in Klipper.
I guess one could argue that the MAXxxxxx chips are actually ADCs (one could even argue that the ADXL345 chips, the MPUxxxx accelerometers, and hall angle sensors are just elaborate ADCs). I’m not sure how it helps to build an abstraction around ADCs when ultimately one wants to go from the raw chip values to a temperature though.
Admittedly, it’s always difficult to make judgement calls when it comes to making code abstractions. Ask five engineers what the best way to abstract something is, and you’ll likely get a dozen different answers.
Cheers,
-Kevin
This. Because of the high frequency read code in C, this feels like something we should unify. Klipper has unified code for writing to steppers at a high frequency. This has kept the C code footprint small and the performance high. Now we are trying to do high performance reads in C and this is seeing the C codebase grow quite a bit for each sensor. Seems like the spirit of Klipper would be to “dry” out this code to 1 read loop implementation.
Just thinking about how this might work; we could replace this switch case structure with a call to a function pointer that’s in a struct that’s looked up by OID. I could something like this:
struct sensor_reader {
void (*read_sensor)(struct sensor_reader *sensor_reader
, struct sensor_sample *sample_out);
uint8_t sensor_oid;
};
Each sensor would need 2 OIDs, one for this struct that points to its read method and another for its private tracking variables, like the SPI device, pins, state etc. The ADXL is a little more complex with its FIFO queue and x/y/z measurements so the sensor_sample couldn’t just be a container for a single integer and a time. But I don’t think that’s insurmountable.
Anyway, I’m keeping it simple for now and just sticking to measuring ADCs using the existing pattern.
Absolutely. I’m getting the hardware to test (on order) but my plan is to wire up the PT100 directly to the MCU so I don’t have to solve this problem to use it. We would only need to solve it if we wanted to support installing klipper on a Prusa MK4.
FWIW, I think we will see a lot of HX717 equipped boards but hopefully not with this dual channel configuration. The HX717 sensor is cheap (<$1USD) and probably just as easy to get as the HX711. I’m going to remove the ADS1263 code from the final PR as I don’t think we will ever see a commercial board with that sensor because its so expensive (~$13). But having it now helps keeps me honest.
I’ve updated the graph that logs the raw data from the sensor to show the errors as vertical red bars and the time error as a green line:
Now I can see what was in the logs in real time.
angle.py has the concept of a “duplicate” reading. This is the most common error and really the only one we could do anything about. The sensor tells you (in some way) that its not ready to give a new reading yet. I call this a sample_not_ready error. Because the clock in the sensor and the MCU are never going to be perfectly aligned this always happens on some timescale.
This really bugs me. I really think we should re-try the sensor to get that reading. Especially for a probing application, a re-try would improve accuracy at the critical moment.
A re-try would have to happen at a frequency higher than the sample frequency to be of any use. This has the downside of increased MCU load when a sample_not_ready error occurs. If I keep the existing monotonic sequence of wakeup times this load would be repeated for every wakeup as the clock drifts.
The alternative is to go to sleep for the full rest_ticks after a successful reading so the next reading is aligned with the sensors clock. But then the sequence of readings isn’t tied to the monotonic sequence and the clocks need syncing.
These events are rare. I could send back a synchronization frame when they happen with the 32 bits of clock. Just reserve tcode == 0xFE as the code for a synchronization frame and put the time in the payload. The next data point reported in the sequence was read at that exact clock time. The payload is already 32 bits so the payload size stays constant. I know I can use clock32_to_clock64() to convert that to chip clocks.
Seem like a sane idea?
I’m not sure what you are referring to here. It is true that the sensor_angle.c code can report an SE_DUP error for the tle5012b, but that error check was only added for completeness - it will never occur in practice as the tle5012b produces samples much much faster than we query.
As long as you are querying faster than the sensor produces data, then I don’t see an issue. The code can still sleep its normal time and when it awakes it will obtain the next sample - no sample will have been lost. The only thing it adds would be a trivial amount of additional latency. Maybe I’m missing something though.
If it’s clock accuracy of the measurements you are looking to improve, then you don’t need to change the protocol. The existence of sporadic SE_DUP indicators is enough information for the host code to determine the actual chip query rate. (You can look at how the adxl345 code synchronizes the clocks as an example.) Indeed, if you’re looking to obtain very accurate clock synchronization for adc samples then you can optimize the existing tcode to do that. (The timing code in sensor_angle.c code was built to handle sensors that take measurements much faster than we query, where as the sensor_adxl345.c code was arranged for sensors that take measurements at a slower rate than we query). I can elaborate, but I’m not sure if increased timing accuracy is what you are looking to obtain.
Cheers,
-Kevin
2 Likes
Ahh HA! That clears up some confusion on on my part 
I’m querying at exactly the same rate that the sensor produces data. 10Hz or 80Hz for the HX711. Up to maybe 4kHz. (Some sensors can do up to 32kHz, like the upper limit of the adxl345, but I don’t think that is going to be worth the overhead for this application.)
I’m trying to keep the compute cost on the host as low as possible so I don’t want to send back a large number of messages. Oversampling with the angle.c protocol would do that (SE_DUP ++). I want to be able to run 5x of these sensors at the same time through an entire print in a tool changer. (likely sensors will be on CAN tool boards so MCU load is less critical).
The simplest implementation would be to make tcode 32 bits wide and just send back the clock with every sample. But of course that wastes bandwidth.
As a middle ground I’m suggesting sending a the full clock only when the sensor tells the MCU that its not ready and it has to be re-sampled. Re-sampling would occur at a higher sample rate so the reading isn’t lost (similar to how endstop re-sampling works). My testing indicates that we would end up with 1 of these events per second or less, often a lot less.
There is a case where rest_ticks is configured too short and you get clock sync frames for every sample. That’s easy to detect and log as an error. This is similar to your comment about SE_DUP not being possible but there for completeness. It should only happen when someone is developing support for a new sensor.
I looked at the adxl345 code and my understanding is this is a distinct use-case. Taking the sample from the ADC resets its internal sample clock to 0. The adxl345 puts samples into a fifo buffer at its own pace and pulling from the buffer has no impact on its internal sample clock. (At least that my impression from the python and c code) I think what I’m suggesting could be a lot simpler than the adxl345 code.
Cheers
