Chscanf, streams, and unget

[alexclewontin]

Hi Giovanni,

I've really been enjoying using ChibiOS. A couple of times, I have found myself wanting to parse streams using a lightweight scanf-like function in the style of chprintf, so I have written one. I searched the forum and saw that a few years ago when the idea was raised you had said you might be interested in including it, so I figured I'd offer it to you. If you don't want to include it in the main trunk for whatever reason, no problem.

The code for actually converting characters to values is pretty straightforward. The big challenge with scanf is that you need an

Code: Select all

unget(stream, char)

function, which replaces char at the front of the stream, so that the next call to get (or read) returns char.

Temporarily I have been using a BufferedStream class which wraps a BaseSequentialStream, and also implements the BaseSequentialStream interface itself, adding a unget function to the vmt, and corresponding macro. It has a couple of disadvantages though:
- The get method of the underlying wrapped class is not the same as the get method for the wrapper, as the former bypasses the buffering layer. This makes it more complicated for the user, and the requirement that the user initialize the buffering layer also makes things more complicated.
- The buffering is often redundant with functionality already in implementations of the BaseSequentialStream class (every implementation of the interface I could find in the HAL already used a buffer of some sort), and therefore carries some unnecessary space & time inefficiency.

The better way to do it would be to implement a pure BufferedStream interface that the relevant classes could inherit from. While thinking about how to tackle the problem, I put together a simple BufferedStream that inherits from BaseSequentialStream, adapted the Memory Streams class to inherit from BufferedStream (which was a pretty simple change), and put together unget functions for the HAL Queue and HAL Buffer Queue modules that could be used to implement the functionality for HAL Serial and HAL USB Serial.

I can see a couple of different options for getting the Serial and USB Serial drivers to implement the interface (multiple inheritance, adding the interface into the class hierarchy between the BaseSequentialStream and BaseChannel interfaces, directly modifying the BaseSequentialStream interface) but before I did more work on it, I wanted to check with you about whether this is something you would be interested in, and see which strategy you thought would be best.

Thanks,

Alex

[Giovanni]

Hi,

Goog work, implementing an "unget" into the stream interface is not possible, it can (and it is) also used for streams where doing an unget is not possible, hardware FIFOs for example.

I think the best option would be to create some kind of unget stack inside the chscanf itself and let it use normal streams, this stack does not need to be very deep I think.

Your "get" function would first look into the stack for data then fetch from the stream if the stack is empty.

Giovanni

[alexclewontin]

I think the best option would be to create some kind of unget stack inside the chscanf itself and let it use normal streams, this stack does not need to be very deep I think.

Your "get" function would first look into the stack for data then fetch from the stream if the stack is empty.

Giovanni

If I am understanding you correctly, I think this essentially what is done by the buffering wrapper layer. My concerns about integrating it directly into scanf are twofold:

First, while you could easily have the unget stack persist between calls, it wouldn't truly implement unget functionality. If there is an ungotten character left in the buffer, scanf would see the stream as having a different state than streamRead or streamGet. These two calls should be equivalent, but if the buffering is inside chscanf, they are not guaranteed to be:

Code: Select all

chscanf(foostream, "%1c",  &c)
streamRead(foostream, &c, 1)


Second, streams need to remain distinct. If foostream has the internal buffer "foo" and barstream has the internal buffer "bar", the call sequence

Code: Select all

unget(foostream, 'a')
char_bar = get(barstream)
char_foo = get(foostream)


should result in {char_bar == 'b' and char_foo == 'a'}, not {char_bar == 'a' and char_foo == 'f'}.

You can begin to mitigate this by maintaining an unget stack per stream, but this raises its own set of problems (predicting number of streams at compile time, associating stream pointer to stack, chscanf cannot know lifetime of streams).

implementing an "unget" into the stream interface is not possible, it can (and it is) also used for streams where doing an unget is not possible, hardware FIFOs for example.

Giovanni

Yes, this totally makes sense. I would then say this is a good use case for multiple inheritance. BufferedStream could simply extend BaseSequentialStream and then things like SerialDriver could inherit both from BaseAsynchronousChannel and also from BufferedStream.

This would leave all the pure interfaces untouched (so as to not break anything). Classes like SerialDriver would be extended, but it should be a non-breaking change, as it would only add to the existing struct, and not change anything existing within it.

[Giovanni]

Couldn't this stack structure part of the chscanf() call context? an automatic array and pointer as automatic variables. The state would not persist after chscanf() returns.

The unget() feature is only needed within chscanf().

Giovanni

[alexclewontin]

unget is only needed within chscanf, but the effects need to be visible outside chscanf.

For instance:

Code: Select all

char buffer[] = "foo123bar";
int res;
char remainder[8];
MemoryStream ms;

msObjectInit(&ms, buffer, sizeof(buffer), 0);

chscanf(&ms, "foo%d", &res);
streamRead(&ms, remainder, 8);


Here, remainder should equal "bar", not "ar". chscanf needs the unget function because it will consume the 'b' before realizing it is no longer getting decimal digits and needs to return. Thus, it needs to put the 'b' back.

The same behavior could be achieved with a peek function which looks at the next item in a stream without consuming it. If you think that would be easier, I'd be happy to try to put something like that together.

[Giovanni]

Also a "peek" would not be possible in some scenarios...

Giovanni

[alexclewontin]

Right, it is essentially two framings of the same problem: how to see what is in a stream without consuming it.

I've attached an example of the backwards-compatible multiple inheritance solution, which I think is the most elegant.

[Giovanni]

The problem is that doing this for serial driver only defeats the purpose of streams, you could not do the same with USB_Serial, SIO and any other stream implementation.

You may just make a serial_scanf() and keep all the changes inside.

Giovanni

[Giovanni]

I would do the following:

- Create a derivative of Base Stream called ReversibleStream (for example) adding unget() as method.
- Modify the memory buffer class to implement this interface. Implement the unget().
- Create a ReversibleStreamAdapter class which implements ReversibleStream and takes a stream as input parameter in the constructor.
- chscanf() would get a reversible stream as input, not a normal stream. Would work on memory buffers and the new adapters.

Less immediate to use but no impact on other things, which I want to avoid.

Giovanni

[alexclewontin]

I think these patches do what you are describing:

- Create a derivative of Base Stream called ReversibleStream (for example) adding unget() as method.

01-BaseBufferedStream-interface.diff

- Create a ReversibleStreamAdapter class which implements ReversibleStream and takes a stream as input parameter in the constructor.

02-BufferedStreamAdapter.diff

- chscanf() would get a reversible stream as input, not a normal stream. Would work on memory buffers and the new adapters.

03-chscanf.diff

- Modify the memory buffer class to implement this interface. Implement the unget().

04-MemoryStreams.diff

I've included 05-optional-SerialDriver.diff, 06-optional-SerialUSBDriver.diff, and 07-optional-NullStream.diff which are all the same thing as 04-MemoryStreams but for SerialDriver, SerialUSBDriver, and NullStream. I completely understand not wanting to impact existing API: these patches do not modify the existing behavior of those classes at all, so they are completely backwards compatible. On the other hand, they are also completely optional, and the first 4 patches work fine without them if you do not want to use them, or save them for a major release.