CRITICAL: If a register READ takes too long, it returns 0. However, 0 is a valid value for reads such as Stall Guard Result.
VikingVoltage opened this issue · 5 comments
At line 873 in TMC2209.cpp:
if (reply_delay >= REPLY_DELAY_MAX_MICROSECONDS)
{
return 0;
}
The problem with this is that 0 can be a valid value. For instance, if reading Stall Guard Result, and the read takes too long, the Stall Guard Result will report as 0 when its actually not that. This will trigger a false stall.
The scenario in which I discovered this: Using esp32 to communicate with TMC2209. The esp32 is also setup as a web server. Serving a number of large files consumes time and prevents the TMC2209 from making a quick read. I could regularly reproduce the error.
Yes that is a good point. What do you suggest would be a better way to handle it? Many other values might be valid as well. Would you like to be able to specify the return value if the reply takes too long? Would it help if REPLY_DELAY_MAX_MICROSECONDS was a value the user could change? The function is blocking and the code would lock up if it never receives a reply so that is one way to wait, but not forever.
It is bad form that the function blocks, I would much prefer to write it as a non-blocking function, but then the user would need to handle the waiting for the reply before reading and handling the data. The single wire UART makes the timing tricky and the code uglier than I would like. At some point I may add a non-blocking option and let the users handle waiting and error handling if they prefer.
Let me get back to you in a couple days.
I was able to get back to this days sooner than expected.
I looked through the Register Map (Section 5) of the TMC2209 Datasheet (Rev. 1.09) (https://www.analog.com/media/en/technical-documentation/data-sheets/tmc2209_datasheet_rev1.09.pdf).
I could have overlooked something, but from what I saw, there is only one READ register that can return a negative value: MSCURACT (section 5.4). It can return a min value of -255.
In the TMC2209 driver, I see ADDRESS_MSCURACT defined in the .h, but doesn't appear to be used(?)
The TMC2209 Driver's read function currently returns a uint32_t.
uint32_t TMC2209::read(uint8_t register_address) {....}
Changing the return value type (int64_t?) would allow the read function to return negative values. And given that MSCURACT would return a min of -255, the read function could return -999999 or something like that to indicate a failed read.
The other option is to return a large positive value that's larger than any possible valid result. I can't quite make out what the largest value to be returned is...the register IOIN (bottom page 24) appears to be the largest at 32bits....most everything else is 24bits or less.
I am no code veteran, consider myself more of a hack, but from my learnings, non-blocking is king. If you decide to dive into a non-blocking version, I'm assuming there are examples out there. I started using PsychicHTTP recently which may offer some ideas. Your code, and Psychic's are beyond my pay grade...so I can't be of much assistance in regard to non-blocking.
I hadn't considered changing the reply delay max time. I may tinker with it. As of now, I can't answer yes or no as to whether or not it would be convenient to be able to change the max delay time.
So what do you want to do in your code when the result tells you that a read took too long?
The default reply delay max time is pretty arbitrary. Can you try increasing that number in the library, recompiling, and see how large it needs to be to work in your application when your webserver is serving large files?
I've been occupied and haven't had a chance to get back to this yet. But I will. In the meantime, I'm having the function return 999999 in the max delay case and testing for it when I check SG_RESULT.
stallGuardResult = getStallGuardResult();
if(stallGuardResult == 999999) {
Serial.println("SG Result Read took too long, so we are skipping this stall guard check.");
return;
}