Receiving poll events in the sigio callback for UartSerial

[darbyShaw]

Type: Question/ Suggested Enhancement
Hello all,
I use mbed OS 5 for nucleo-f401re.
I want to use UartSerial asynchronously using interrupts.
I see that UartSerial by default attaches to the rx_irq callback for RxIrq.
I attach my callback with the sigio function. My callback is called when wake is called from rx_irq. But since I have no way of knowing that whether my callback was called for a read or a write event(since I cannot call the readable and writable functions on my object(they are ambiguous and static cast to the base SerialBase does not work since it is private inheritance)), I was thinking of passing the revents to the callback(_sigio_cb) after poll.
Example:

void rxCpltCallback(short events){
	  uint32_t data_read=0;
	  uint32_t length = PACKET_SIZE+PACKET_HEADER_SIZE+PACKET_TRAILER_SIZE;
	  if((events & POLLIN) == 1)
	  {
		  data_available=1;
	  }
	  else if((events & POLLOUT)==1)
	  {
		  writable=1;
	  }
}
void receive(uint32_t *p_size)
{
			serialPort.sigio(rxCpltCallback);
			while(!data_available || !writable);
			if(data_available)
			{
			serialPort.read((uint8_t *)&receivedPacket[0], PACKET_SIZE+PACKET_HEADER_SIZE+PACKET_TRAILER_SIZE);
			printf("\n\r read data");
			}
			else
			{
				printf("\n\r writable=1");
			}
}
void UARTSerial::sigio(Callback<void(short)> func) {
    core_util_critical_section_enter();
    _sigio_cb = func;
    if (_sigio_cb) {
        short current_events = poll(0x7FFF);
        if (current_events) {
            _sigio_cb(current_events);
        }
    }
    core_util_critical_section_exit();
}

Is this the correct way to do it?
If yes, we would possibly want the events passed to the callback function.

[0xc0170]

cc @hasnainvirk

[hasnainvirk]

@darbyShaw readable() and writeable() being ambiguous is correct. I will send a patch through.
However, you can make it work at the moment by following any of the method below:
Method 1:

// cast your UARTSerial object to FileHandle that's what it really is, a FileHandle
UARTSerial my_serial;
FileHandle *fh = &my_serial;
if (fh->readable()) {
        //do blah
} 

Method 2:

// use poll directly
if (my_serial.poll(POLLIN) & POLLIN) {
       // do blah
}

// OR you could do 

pollfh fhs;
fhs.fh = &my_serial;
fhs.events = POLLIN;

if (poll(fhs, 1, 0)) {
      //do blah
}

Plus, you can't really use a Callback template to set a variable. While constructing a Callback object, you can set a constant but not a variable. So on your sigio(), you don't know whether its readable() or writable(). You must check by poll or by invoking readable()/writable() which are currently ambiguous. Please try the methods stated above while we will prepare a patch for the bug.

[hasnainvirk]

@darbyShaw One more thing came in to my mind, UARTSerial is essentially a serial with buffering you may have UARTSerial writable most of the time. Internally, UARTSerial will take data from buffers and write to the peripheral when peripheral is writable.

[kjbracey]

I've created a PR which resolves the ambiguity. In the interim, the simplest workaround is to resolve the ambiguity by calling serialPort.FileHandle::readable() - basically a neater version of method 1 above.

The strange thing is, I recall this ambiguity popping up during development, and thought we had already put some explicit overrides in UARTSerial to avoid it.

[kjbracey]

(This escaped our attention because our code working with UARTSerial doesn't directly use the UARTSerial type, but is all written using FileHandle pointers, making it generic - see https://github.com/ARMmbed/mbed-os/blob/master/platform/ATCmdParser.cpp and https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_LWIP/lwip-interface/ppp_lwip.cpp ).

[darbyShaw]

Thank you so much for helping me out! I'll try using the FileHandle.

[darbyShaw]

I guess I also want to be able to attach my IRQ handler for receive on UartSerial which is not possible. I am thinking of using the Serial interface since UartSerial doesn't really help for higher baud rates for packets(even of size 128). I keep missing some bytes or read some invalid data.
I will try out https://developer.mbed.org/cookbook/Serial-Interrupts
But this is weird. It outlines an Rx interrupt handling routine that performs a getc() that takes locks which is not allowed in an ISR. And I cannot do a __base_getc() on it since it is protected. I guess I have to use just the SerialBase which is as good as using the STM HAL for nucleo as is.

[kjbracey]

UARTSerial will call your sigio (from interrupt context) whenever data has become available. It will be draining data from the serial port under interrupt. There's no reason your interrupt handler would do a better job.

Your responsibility is to make sure your reading thread drains the buffers fast enough in response to that sigio. If you don't, then data would be lost. Depending on the behaviour of your application, you might need bigger buffers.

There is a prototype extension to UARTSerial adding HW flow control, which could stop the sender if you don't read fast enough (and if the HW flow control lines are wired). See here: #4428

I think @RobMeades is happy with that change, in which case I should make a PR for it.

That may be required because some platforms can't actually read from the port under PIO interrupt fast enough to keep up with line rate - see #4722

[RobMeades]

@kjbracey-arm: yes, happy with that HW flow control change, making a PR would be good.

[darbyShaw]

@kjbracey-arm
Okay my reading thread just calls read on the handle when I my callback is called i.e. I don't read it directly from there because of read using a lock and I can't use it from interrupt, I signal it to another part of my program that waits for this signal and call a read on it. How can I make this faster?

[kjbracey]

Waking a thread to do the read as soon as you get sigio should be plenty fast enough, at least at baud rates like 115,200. Are you making sure you read everything available every time you wake - read until you get EGAIN?

The lwip PPP implementation I linked to above manages fine sending and receiving 1500-byte IP packets via a 256-byte serial buffer at 115,200 baud - sigio calls stream_cb which queues ppp_input on a normal-priority EventQueue thread. That calls read until EAGAIN.

Do you even need to be non-blocking? If you have a dedicated thread, maybe you could just do blocking reads, and not have a sigio handler.

[darbyShaw]

I do read everything, that I am sure. I will look into lwip implementation.
I can't use a blocking read since it really slows up things and even when I was doing it I was missing some bytes with overwrites I guess. Is there way to use our own buffers instead of the circular buffers used?
I tried using the read from SerialBase which is an async_read using interrupts. I receive the first 128 bytes better than I received with above methods but after that it just locks up and I cannot read anything and things I write are not seen at the sender at all. I guess I am getting locked up somewhere. Let me check this again.

[kjbracey]

Blocking reads may not be appropriate for your application - it would mean a dedicated read thread that didn't have other stuff to do. But a blocking UARTSerial::read would only block that thread - it will sleep properly and it won't slow down other threads.

That's unlike Serial, which is a totally non-buffered, spinning implementation - any read from Serial when the device isn't readable will keep that thread running continuously, guaranteeing 100% CPU utilisation and slowing other threads. You'll also note that Serial::read(100) will spin until all 100 bytes is available. It won't give you partial data, meaning it's never really safe to do a multibyte read. UARTSerial::read(100) will block until at least some data is available.

You could certainly create your own interrupt-based buffer-driven implementation on top of Serial/RawSerial, and lots of people have, due to the inadequacies of Serial, but I don't see a reason to expect them to perform better than UARTSerial. The only 2 areas of potential improvement I'm aware of are:

• using async RX if available, so more than 1 byte per interrupt (I have a prototype patch for this, not a PR yet)
• adding HW flow control (now pending as UARTSerial: add flow control and format APIs #5088 )

Now, at really high line rates without flow control, it's possible Serial works reliably but UARTSerial doesn't, simply because Serial can devote 100% of foreground CPU time to reading the serial port - UARTSerial reading by 1 byte per interrupt in the background may not be able to keep up. (The catch is that Serial would drop stuff in between read calls as it can't buffer).

What data rate are you actually using here? If it's 115,200, I think you're driving it wrong. If it's more like 1Mbps, then you probably need the flow control, and would benefit from testing an async RX patch.

[kjbracey]

Do look at the lwip_ppp.c implementation - that's a more-than-textbook working example of non-blocking event-driven operation. From the other side, AtCmdParser shows it in blocking-with-timeout operation.

[darbyShaw]

Okay. I am trying to use the FileHandle approach used in lwip_ppp.c I can now receive 128 bytes but after receiving them, I don't get the next 128. Basically I am trying to implement a simple YModem protocol with ACKs and stuff. When I did it with read_async, I ran into an Overrun error which I realised after I put a print with semihost in the UART_Error_Callback.
Is there a way to know about Overruns with UartSerial?

[kjbracey]

Error indications aren't in the base target serial HAL API, so aren't available in UARTSerial.

Why is the number 128 significant? Is it the buffer size? Or is it the amount you're reading?

Maybe you're reading 128 bytes, and waiting for the next sigio? But you never get another sigio because you didn't actually empty the buffers? sigio only happens when the FileHandle becomes readable (ie changes from non-readable to readable). But that means you have to empty it first.

Do be aware that putting serial-based printfs into a program accessing another serial port is a very good way to guarantee you lose data. "stdout" is totally unbuffered, and will probably block the thing that's supposed to be reading from your serial port.

[darbyShaw]

128+header+trailer(3+2) is the packet size for YModem protocol.
I already have a working app with the stm32_hal used raw with HAL_UART_Receive_IT. It works even with printfs to another serial and without retransmissions and no flow control. Anyway, I disabled prints and have only one print in the UART_Error_Callback. I tried with baud rates: 9600, 115200 and 921600.
My HSICalibration value is the default 16. I tried increasing it to 22 to no avail.

This is how I do it:
`UARTSerial serialPort(SERIAL_TX, SERIAL_RX, 115200);

void rxCpltCallback(void){
FileHandle *fh = &serialPort;
if(fh->readable())
{
trace_printf("\n Data available");
data_available=1;
}
}

int main(void)
{
while(!data_available)
{
serialPort.sigio(callback(rxCpltCallback));
t.sleep(1);
if(packets_received==0 && !data_available)
{
//trace_printf("\n asking for a packet");
uint8_t askForPacket=CRC16;
serialPutByte(askForPacket);
}
}

if(data_available)
{

	//trace_printf("\n\r reading data");
	for (;;) {
	ssize_t len = serialPort.read(&receivedPacket[0], PACKET_SIZE+PACKET_HEADER_SIZE+PACKET_TRAILER_SIZE);
	//trace_printf("\n data read = %u", len);
	if (len == -EAGAIN) {
		break;
	}
	}
	//do something with data
	data_available=0;
}

}`

[kjbracey]

• sigio() registers the handler, and only needs to be called once
• If a "packet" does arrive in multiple reads (very likely), you're not adding together the lengths or moving the data pointer for each bit
• If you see EAGAIN partway through a packet (because you're reading it faster than it's coming in), you won't pick up the rest later.
• Is that a 1 second sleep? So you maybe don't empty the buffers for 1 second after it arrives. You should be using some signalling mechanism that wakes up immediately from the sigio callback like an EventFlag. Although maybe the wait for 1 second is required to give the "read-all-at-once" code a chance to work?
• With code structured like this, which is a thread in a processing loop, you don't need the sigio handler at all - if you want to wait for data, use poll() from mbed_poll.h with a 1-second timeout, like AtCmdParser does. (This works like the standard POSIX facility).
• What buffer size is in use here? The default 256?

[darbyShaw]

Okay. Thank you for those pointers. The buffer size used initially was the default 256. Here's what I tried: I removed sigio() altogether. I removed sleeps and I used the poll system call to get each packet this way:

int lengthRead=0;
int pollAgain;
serialPort.set_blocking(false);
pollfh fhs;
fhs.fh = (FileHandle*)&serialPort;
while(lengthRead < PACKET_SIZE+PACKET_HEADER_SIZE+PACKET_TRAILER_SIZE)
{
		pollAgain=0;
		fhs.events = POLLIN;
		count = poll(&fhs, 1, 1000);
		//trace_printf("\n count = %d", count);
		if(count>0)
		 {
		        ssize_t len=0;
			while(len>=0)
			{
		         len = serialPort.read(&receivedPacket[lengthRead], 
                                                   PACKET_SIZE+PACKET_HEADER_SIZE+PACKET_TRAILER_SIZE);
			if(len>=0)
			{
					  lengthRead+=len;
					  //trace_printf("\n data read = %u", lengthRead);
					    		
                             if(lengthRead>=PACKET_SIZE+PACKET_HEADER_SIZE+PACKET_TRAILER_SIZE)
			     {
			    			lengthRead=0;
			    			break;
				}
			    		}
			    	}
			    	if (len == -EAGAIN) {
			    		pollAgain=1;
			    		//trace_printf("\n read again");
			    		//continue;
			    		//break;
			    		}

		    }
          }

I think this should work, but I still don't get the entire packet.
One more question I had is if I switch back to read async with the Serial class, is there a way to handle overruns?
I need the baud rate high since I have an entire file to transfer that can be of size 96k-128k.

[kjbracey]

I wouldn't think you need to handle overruns or any other serial error specially - I would expect it to be sufficient to spot the CRC failure or 1-second timeout which arises from such a loss, and then send a NAK to cause a retransmit.

On the read, you should at least be setting the read length to (buffer size - lengthRead).

But for YMODEM you should be reading the amount indicated by the block length marker. My copy of the spec seems to basically say "wait 10 seconds for a start byte, either SOH or STX". Once you have that, you should then be reading 2+128+2 or 2+1024+2, depending on which byte it is, with a 1-second time out.

If your buffer size is 256, and you're using 128-byte packets, I don't really see what the problem could be. The entire YMODEM packet will always fit in the buffer, so it doesn't actually matter how slowly you read it. Is it that you're getting 128 bytes of data total, or just 1 128-byte packet (so 133 total?).

Are you actually sending the ACK so you get the second packet?

[kjbracey]

That code as written seems like it can never terminate - you have an inner while loop you don't need, and if it ever sees a whole packet, it breaks out from the inner loop and sets the length count back to zero, leaving you to go around the outer loop again.

As you're no longer using sigio, you don't need to read until you get EAGAIN (which you were doing to make sure you'd fully processed the sigio callback) - you can just do one read per poll

while (dataRead < expected) {
     if (poll(dataRead == 0 ? 10 seconds : 1 second)) {
          dataRead += read(remaining expected data)
     } else {
          break; // timeout
     }
}

if (dataRead < expected || CRC failure) {
   send NAK and retry (or send CAN and give up if failed too many times?)
}

[darbyShaw]

Ah yes. Actually I had written it without EAGAIN initially without nonblocking set to false. So I didnt have the inner while loop. I just added it to see if it worked with non blocking. I will check that. But with blocking and poll it just gave me an event for the first time after which revents never had POLLIN set and poll doesn't return > 0. I basically receive partial data. I will try out what you suggested and get back. May be I am missing something trivial. My packet size is 3+128+2 = 133

[kjbracey]

If using poll, you shouldn't need non-blocking - if poll says it's readable, then that /should/ be a guarantee that the next read will not block. (Assuming a working FileHandle that doesn't magically become non-readable). EAGAIN should not be possible on the first read after poll indicates an event.

I'm still not clear what you mean by "receive partial data". What YMODEM block size are you using? 128 or 1024?

• Are you getting one 128-byte block, and not the next, which means you're not acking correctly - the other end just is not sending the second block?
• Are you getting 128 bytes of a 1024-byte block? If the buffer size is 256, I don't see how that's possible - the UARTSerial will get at least 256 no matter what you do.

[darbyShaw]

I use ymodem block size 128. I fail to get the first block itself so ACK is not an issue.

I am not sure about this but I think I see an overrun before receiving the entire packet even at baud 9600. I see it in serial_api.c in serial_readable(serial_t *) which is called with SerialBase::readable().

[kjbracey]

It could be your platform has some interrupt latency problem meaning serial port reception is never reliable. If a driver is disabling interrupts too long, the serial port is usually the first victim to suffer.

To get problems at 9600, it would have to be pretty bad though.

Do you have any other code or drivers active that could be interfering? If so, try deactivating it.

Previously you said you got the first 128, but not the second 128. Can you be more precise about the situation now - you now say you're not receiving the first block. How much of the first block are you getting? Precisely? Are you losing a couple of bytes of the 133?

I see you're using an STM32F4. I know we have seen problems with that chip - see #4722, #4734 and #4780. Do you have a version of mbed OS with pull #4734 integrated?

Also note that the STM32F4 has NO hardware serial FIFO - only a single-byte data register. This means it is much more susceptible to overrun due to interrupt latency than most other SoCs. It may be that you are compelled to use hardware flow control at higher rates sooner than you would be on other devices.

That in turn requires this pending PR to give you access to hardware flow control with UARTSerial: #5088, and you obviously need those lines wired up and working to whatever you're talking to.

[darbyShaw]

Thank you so much for your time and prompt replies. Mbed-os has got awesome response time for support.
I was able to complete my application with SerialPort Interface's async function

Serial serialPort(SERIAL_TX, SERIAL_RX, 500000);
`serialPort.read(&receivedPacket[0], PACKET_SIZE+PACKET_HEADER_SIZE+PACKET_TRAILER_SIZE, rxCpltCallback);`

I handled some occassional overruns with a Ymodem NAK to the transmitter and everything started working in a flow.
I appreciate all your help!
If I can spend some time, I will try to do the same with UARTSerial with flow control and get back.

[0xc0170]

@darbyShaw Is this considered resolved?

[darbyShaw]

@0xc0170 Yes resolved using the Serial interface. Thank you.

[ciarmcom]

ARM Internal Ref: MBOTRIAGE-179