I don’t think it’s that interesting. There’s one division operation in this code, and I already demonstrated one way to rework it to avoid that.
Did not know that even simple integer division is so problematic on microcontrollers. If that was floating point emulation, I would have immediately assumed that this was the issue.
I already fixed that. Simply splitting this into
uint8_t next_sensor = (current_sensor + 1);
next_sensor %= spi->sensor_count;
works without problems. But a plain if or ?: was the next step.
Edit: Found a discussion thatt a possible overflow causes the use of this function.
Searching for “__udivmodsi3_internal” yield only 2 other hits which directly relate to changes in gcc, maybe somebody more experienced can have a look there.
The python issue is also fixed. In Python2 you cannot use ° even in comments.
But now the PR build fails on an rom segment overflow, tried something simple, but that did not work. Haven’t looked futher sofar.
Edit: I made a standalone Container for running the CI-tests, but this completes without any overflow issues: Dockerfile for klipper ci-tests · GitHub
As above, you really want to avoid both division and modulo. They are very slow and significantly increase the code size. If the above worked, it fixed the symptom and not the underlying problem. Replace with an if (or equivalent ?: ).
Cheers,
-Kevin
1 Like
Ah, now the failure (and its fix) in Bulk ADC Sensors by garethky · Pull Request #6555 · Klipper3d/klipper · GitHub are potentially relevant.
Don’t know if this is against the rules, but since multiple people seem interested in what Kevin said… Here’s a good video on the topic.
In my PR I did not take on the multiplexing problem but its something I’m aware of. Prusa is doing a channel switching to get the filament sensor readings on channel B in their load cell equipped printers.
We could do multiplex switching on the MCU and tag the data as it is recorded. So {channel:value} pairs. That could be 1 byte for the channel and 4 bytes for a value.
The “sampling plan” could be explained to the sensor as a list of {channel:n-samples} pairs. A plan might look like:
- Sample Channel A 20 times
- Sample Channle B 1 time
- Sample Channle C 1 time
And the sensor would do that in a loop forever.
But that is the easy part. Its what happens back in python that I didn’t want to put in my PR:
- Data needs to be routed out of the sensor to different python modules. Maybe Channel A is connected to a load cell and channel B to thermistor and channel C to a filament sensor. Each one needs its own data feed, which is doable. The config also needs to let you reference the sensor in all these places. So that seems to mean the sensor would have to be config objects, like the ADXL345. This is something kevin wanted to avoid. So I don’t know how you would set this up.
- There needs to be a way to get the sensor to change its sampling plan at runtime. This is literally what Prusa does, they shut off the filament sensor data feed when they home or probe. But that is crude. I wasn’t looking forward to implementing that and trying to figure out which module could demand exclusive access to the sensor. Shutting off a feed to a Filament sensor is pretty benign, but what happens if you shut off the data feed to a heater? No one should build a printer like that, but someone is going to try and might burn their house down.
- The timing of the data is likely incorrect when switching channels. Most ADCs have some additional time required to switch channels. This would break assumptions of bulk sensor that reads happen at perfectly consistent intervals. You might have to just live with this as there is no easy fix.
- Sensors usually have a settling time, in samples, when the channel is switched. So it can be that the first few samples need to be discarded. The ADS1118 has Singe-Cycle settling but its not universal. So the config might let you do something the sensor doesn’t support leading to weird results.
I’m not opposed to coming up with a more general design but I am not going to work on it if its not welcome in the code base. I’m trying to get load cell probing to work, this is just a “side project”.
To be clear, my motivations for inserting myself into these discussions are purely because of these factors:
1 - I have (good enough?) support for the ADS1118 specifically and have attempted to implement it in a way that should be usable with some changes for other printers and non-hotend uses.
2 - The user base for my repo (targeted for Makerbot Rep2(x) and clones) is not large enough for all of the changes to be pulled into mainline.
My desire would be that if ADS1118 support is in mainline it is implemented in a way that natively supports my makerbot use case or at least is easily extensible to support two type k thermocouples for hotend temperature sensing. I am more than happy to work on changes needed to support other printers and/or use cases to further getting any of my changes into mainline.
To add to your bullet points:
-
I struggled for quite a while with how config should look and how the code should work with multiplexing. The hotend use case has another dimension in that the raw data isn’t usable as is, it needs two of the three channels and then a computation needs to be done. In the end I decided (with guidance from others) that there needed to be a config section for the ADC to handle the communication with it, and to use pins to represent the various channels.
-
Prusa’s approach appears to be driven by cost cutting? i.e. we have a left over adc channel, let’s add filament sensing. Agree that any implementation of this should specifically error out if temperature sensing is shared with anything else. I’ve not thought at all about changing the “mode” of multiplexing as you describe but I can see how it would be done in the mcu code.
-
Agreed - and these concerns are part of why support for these sensors has to have some mcu code to coordinate this.
-
Agreed - same comment, this needs to be handled by mcu code.
@garethky When adding my module I ran into the problem you have where the image is too big for the mcu. I added a kconfig specifically for ADS1118 and defaulted it to no in the following mcu make files:
avr
ar100
stm32f042
Maybe this helps if you haven’t already sorted this out.
Yes. Logically you can turn off the filament sensor when not extruding so this makes good use of an existing piece of hardware. But the programming is more complex. Seems like the same thing is going on in the Flashforge Creator Pro2.
For the ADS1118 I don’t think 16 bits is going to be useful for load cells. We have done the math and several people have tried and we just don’t end up with a usable range. You need 24 or more bits. So for my part I’m happy to not make this a bulk sensor because I don’t think its useful in that way.
So I’m happy not to try and merge the two PRs.
So, it think i got everything working. The regression tests are green.
I had a short look at dockterj’s ADS1118 implementation.
My firmware implementation is smaller and dumber.
- Mine just reads values from the inputs and the temperature sensor, no interpretation, just reporting back
- I think his can be reconfigured or queried on demand, mine is configured at the beginning then loops over the configured sensors forever and reports the readings to Klippy without requests, just unidirectional communication
I thought that the ADC1118 usage was unique to the FFCP2 so I had no other use cases in mind.
The integer division is gone, the build excluded on stm32f042 like in the mentioned PR.
It may be useful to make the thermocouple calculations part of the library, seems there are more users.