[MODSERIAL] Resize Buffers Problem

10 Dec 2014

Hi! I am using the latest MODSERIAL library from "Erik Olieman".

I have tried the Buffer Resize Example from Cookbook but there are some bugs. First of all: The BufferOversize (shouldn't its name be BufferUndersize?) is thrown at the false condition. size is new buffers size, buffer_count[type] is current buffers content size, which should fit in the new buffer.

RESIZE.cpp, line 58

if (size >= buffer_count[type]) {
    return BufferOversize;
}

// should be

if (size <= buffer_count[type]) {
    return BufferOversize;
}

After this change, the resize returns no error code. But with a filled buffer, it does not work. If I start transmission of some bytes and immediately calling txBufferResize and adding some more bytes, the output is completely rubbish.

Here is my test program with the MODSERIAL library and the "<= >= fix" (to avoid BufferOversize Error): http://developer.mbed.org/users/BlazeX/code/Test_MODSERIAL_BufferResize/

In short: this source code is transmitting "DHello World! newÝõ!T"

...
MODSERIAL pc(USBTX, USBRX); // tx, rx
...
    pc.printf("This is a little test text.");
    pc.txBufferSetSize(32);
    pc.printf("Hello World!");
...

The sourcecode looks like it would stop Interrupts, to prevent buffer modification. And the old buffer gets coppied into the new buffer, the ring buffer cursors are getting updated too. But something goes wrong.

Erik, please help!

10 Dec 2014

For fixed issues, just submit a pull request and I accept it :) (Or I add you to devs).

Those are parts of MODSERIAL I have never looked at :P. Anyway quick scan gives what is probably the issue:

do {
        c = __getc(false);
        if (c != -1) s[new_in++] = (char)c;
        if (new_in >= size) new_in = 0;
    }
    while (c != -1);

It uses _getc to copy the old buffer into the new one. Which is all great fun, minus that it is completely unrelated to the TX buffer you are copying. Although I would expect its RX buffers to be empty, so it should be doing nothing, unless you previously sent it something. But for sure something is going wrong there .

11 Dec 2014

Hi Erik! I have added a method called "moveRingBuffer" to move the content from a partially filled buffer (ringbuffer of rx or tx) to a new buffer. The resizeBuffer methods of my newest MODSERIAL revision are now using this methods, but still cause trouble. At the moment, I have no idea why is does not work.

Please have a look at the newest test at: https://developer.mbed.org/users/BlazeX/code/Test_MODSERIAL_BufferResize/

It outputs:

01234567
01234567
01234567
01234567
01234567
034567
12

instead of:

01234567
01234567
01234567
01234567
01234567
01234567
11 Dec 2014

At first glance your implementation is exactly how I had envisioned it too. I wouldn't know directly what is causing it. The LPC1768 has also not a really great UART (one of the few things about NXP peripherals I dont like), so might be interesting to see what happens on another target. I'll try to have a look at it in the weekend.

11 Dec 2014

Ok, sounds good.

When the MODSERIAL library is working, what do you think about a remake for RTOS? Like your RTOS_SPI. :)

Also the official mbed team could do this: RTOS versions of Serial, I2C, SPI: ring buffered, IRQ driven hw interface operation (like MODSERIAL does) and use RTOS to read/write, "while putting the current thread in Wait state."

11 Dec 2014

Possibly, I don't know if it would be as useful though, since Serial is used asynchronous more often than SPI, where with SPI you want to wait until it is done, Serial will just run in the background. And if I would do it, I would also consider using BufferedSerial instead, since it works with all targets, although I don't know if it does everything which needs to be done for RTOS, so maybe MODSERIAL would be better choice :).

Or plan B which I am very slowly working on wondering if I can get it implemented in a nice way: A generic RTOS-wait lib which puts the current thread in wait state until interrupt is received. Pretty straightforward in principle, but to do it nicely in a way where many types of interrupts can be handled (for example MODSERIAL demands a function with an argument for the interrupt) is a whole different story.

11 Dec 2014

Serial running in background.... transmitting yes. But most (of my) serial receivers are protocol parsers, always waiting for next byte(-> waiting getc) and parsing it. The resulting packet (command, whatever) goes than to a queue. That is, what I need Serial for. A Parser-Thread would be perfect.

Yes, common SPI or I2C operation is transmit some bytes, and read the result. E.g. reading sensor or flash memory. The unused time, when the hardware communicates, could be easily used with RTOS.

For plan B you need a really smart implementation. Additional (generic) overhead in Interrupts can become a time problem for high data rates. But wouldn't it be almost as easy to use simple signal_set in the interrupt and signal_wait in the (read/write calling) thread?

12 Dec 2014

It would indeed be very similar to that. But adding RTOS to MODSERIAL would also be similar to that, RTOS_SPI also does something like that. Only it is only available in combination with DMA since as you say interrupt overhead is an issue for high data rates, so if it would fire an interrupt for every byte it would still be largely busy running interrupts.

With serial this is less of an issue since often it is at much lower baudrates. Adding basic RTOS to MODSERIAL should be straightforward enough.

12 Dec 2014

Fixed it! I made a really stupid mistake. This should move the ring buffer content (located in the middle of the old buffer) to the new one:

buggy move

// copy old buffer content to new one
if(buffer_in[type] > buffer_out[type])      
{   // content in the middle of the ring buffer
    memcpy(&newBuffer[0], (char*)buffer[type], buffer_count[type]);
}  

fixed move

// copy old buffer content to new one
if(buffer_in[type] > buffer_out[type])      
{   // content in the middle of the ring buffer
     memcpy(&newBuffer[0], (char*)&buffer[type][buffer_out[type]], buffer_count[type]);
 }  

So it always copied from the first element of old buffer, instead of using the out index. I didn't notice that, because I always sent the same string. *redfaced*

The Test-Program runs fine now!

When you accept my pull request, my own lib repository is not needed anymore. Can I replace it in my test program with the official one and remove my repo without damaging something?

12 Dec 2014

Yes once accepted that will work fine.

Point 1: I made you developer, you can also do that now (or directly modify the library).

Point 2 why I haven't accepted it directly now: There might be one more issue, when quickly scanning it I don't see a size check: what if the old buffer is full, and you memcopy it to a new buffer which is smaller? Then it should verify that it won't overflow outside of the allocated buffer.

12 Dec 2014

The size check is done in MODSERIAL::resizeBuffer. Have a look at RESIZE.h line 43.

To check this case, there is a test in my test-program.

12 Dec 2014

Ah okay, I assumedi n those situations it would just throw out part of the buffer, but the check is already there. I'll accept the pull request then.

12 Dec 2014

Fine!

I have got another idea, the opposite of resizing buffers: optionally using global static ones. I think about an additional constructor, that takes two buffers (pointer and size), or a replaceBuffer method. The buffers can be static (not dynamic allocated), so the memory usage calculation of the compiler recognizes them. What do you think about?

Or is there a preprocessor directive to tell the compiler the additional memory usage? So MODSERIAL keeps full control over the buffers. Like this:

#pragma ram_add("pc serial tx buffer", 256)  // <-- such thing existing?
#pragma ram_add("pc serial rx buffer", 512)
MODSERIAL pc(USBTX, USBRX, 256, 512);

The Cookbook page needs an update too. There are links to Andy Kirkhams old MODSERIAL repository. The complete page, not only a link at the bottom ;)

And the rtos methods.... I think a remake would be good, because MODSERIAL has grown to a big confusing construct. Some things (like the Parity-Enum, or the macros) are not necessary and the buffers used with enum IrqType? Wouldn't an enum BufferType do a better job? I know, never touch a running system.... But the next person, who want's to bugfix something, should be able to understand it.

12 Dec 2014

Thats completely true, which is why I also looked at BufferedSerial as base to give MODSERIAL like functions. Need to see what would need to change there to be able to make a good child library with more functions (currently for example too much is private, but also other issues might arise).

In general even with my modified version MODSERIAL is a bit of a pain in the ass to understand. I filtered out the target specific stuff, but the rest is still imo a bit too extensive.

14 Jan 2015

I proudly present:

Import librarySerialDriver

Buffered Serial Port Driver for RTOS

I combined MODSERIAL's features like isr driven circular buffering and BufferdSerial's and RTOS-Serial's ability to "idle wait" using RTOS. But I don't use extra threads nor target specific functions. It can be used instead of mbed RawSerial.

Dynamic buffer resize is not implemented, because no one but me noticed that the MODSERIAL implementation does not work. So I don't think it is really needed. :D