Fix findUntil and general timeouts in Stream library #2696
Conversation
|
I think the original idea was in fact for the timeout to be per-character, not total. (Regardless, the current behavior should probably be better explained in the documentation: http://arduino.cc/en/Reference/StreamSetTimeout). An overall timeout is not a bad idea, but it would be a significant change from the current behavior. Maybe it could be an additional setting? |
|
If that was genuinely the intent, that is a) not at all conveyed well in the documentation and b) would seem to have fairly limited utility. Making it so that the basic interface provides no means of detecting a device that is misbehaving (ie generating spurious characters because the rx line is floating) seems like awful design. I would be shocked if this were genuinely the original design goal, just that the current implementation was "good enough" to make things mostly work and nobody came back to make them better. I agree that this is a significant change, but based on the documentation and the usage I found online trying to figure out how to use the library, I'd be genuinely surprised if many people thought that the timeouts work the way they currently do. If I'm genuinely wrong about the original intent, the documentation needs to be made very clear of this. That said, the documentation needs to be made much clearer about lots of things. I only looked at the code in the first place because the docs were insufficient and in places gave incorrect information about the parameters of the various methods (treating strings/char *'s interchangeably with chars). Thus once I get this bug fix settled I want to go over the stream documentation and suggest a bunch of fixes but I want to know specifically what the methods are all supposed to do and make sure that they're actually doing them first. |
PR arduino#2696 without timeout-related changes
|
Hi @Xuth , I'd like to merge it as-is but the "global timer" modifications are not directly related to the topic, and will need a lot more discussion (also on the mailing list). So I rebased only the findUntil patch here facchinm/Arduino@f43a7a698 , tested it and it works like a charm so, if it's ok for you, I'd merge only that part and close this PR |
|
Thanks, this looks good. (I don't have that much pride tied up my code. i'm mostly coming at this as a user wanting to be able to use this for my own needs) I need to write up some comments for the mailing list on the timeout issue and also (after using the library a bit more) realizing that at times I wanted access to the timedRead() routine and see no reason why that and the timedPeek() routines shouldn't be made public rather than protected. |
|
Perfect, waiting for you in the mailing list 😉 |
PR arduino#2696 without timeout-related changes
This is my proposed fix to issue 2591 #2591 . Primarily this fixes two things: The most important is that searching for something like "1112" in the stream "11112" now works. The second thing is that timeouts on stream reads are properly respected. In the previous version of the code, if you set a timeout of 1000 millisecs and your stream produced a single character every 500 milliseconds, a read would never time out since the clock would be reset every time the program entered timed read. I had to add a handful of private methods to keep all of the other methods intact but I believe that all of the public and protected methods are maintained as before minus the two bugs I described. I also added a low level find routine that would support an arbitrary number of target strings since it's not uncommon to be searching for more than one good string and one error string indistinguishable in the interface from a timeout. This last routine, findMulti() is not very Arduino library like (requiring you to pass in an array of structs) but you need one integer of scratch space per find target* and I felt that dynamically allocating that in the function was a greater sin with only 2k of ram. Everything else uses a constant amount of stack space.