-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
How to read UART bytes in queue after break? (IDFGH-2424) #4537
Comments
Can you provide the timing diagram of your UART port through a logic analyzer or oscilloscope?Especially before and after the UART_BREAK event. We want to know if there is a UART_DATA event from the UART_RXFIFO_TOUT after the UART_BREAK event |
I have attached a csv file of the data/timing on the DMX input port. (I had to change the file name, just rename without '.txt'. I have also provided the Saleae Logic capture file as well. You can download their software to look at it. (Same with the name, just rename without '.txt'. I have no idea how to capture UART_DATA event. Isn't that internal to the UART and reported by interrupt? |
@luksal @macdroid53 |
DMX protocol is very simple. It starts with a break (the break is a low signal for approximately 88us). It then sends 513, 8bit values (each with 1 start bit, two stop bits, for a total of 11 bits per byte). This is sent at 250k baud. I have no transmit code to offer, I'm working on a receive only application. (though I believe any serial source that could send 513 bytes @250K baud should be sufficient. See my reasoning below...) The following describes what I think I'm seeing: Note the value 120. It is the same as If this description is correct and the driver does not takes steps to return bytes remaining in the buffer when less than the threshold and the break is detected, then any attempt to send an amount of data not evenly divisible by 120 would loose the remaining data. So, any serial source, say another esp32, could send 513 bytes. (I say 513, because the DMX "slots" are numbered 0 through 512, so the protocol actually sends 513 bytes.)) I think the driver knows it has data. It has two variables that imply this: If I read uart.c correctly, this condition would occur when: |
I have been studying uart.c. In the function:
it appears the actual break detect interrupt does nothing but return event.type set to UART_BREAK. So...maybe the break detect needs to check the FIFO > 0 and act accordingly? Or, am I way off base and confused (which is very possible. ;) ) |
Just wanted to check if there is anything else I can contribute to this? |
That sounds right. Now, the fifo is only emptied on threshold or idle timeout but you need it to be emptied on break also. |
@luksal, thanks !! |
@koobest that would add unnecessary latency because the next event could be threshold which would be an extra 1000 bit periods |
@koobest that's not a good solution, because as @negativekelvin says this adds latency. Perfect solution would be the possibility to check whether there are bytes in the fifo left and read them, when the break event occurs. |
My experiments found the remaining bytes aren't there after the new break is detected. As @luksal notes, being able to determine that there are remaining bytes when a break occurs would be the correct solution. |
What if you change this esp-idf/components/driver/uart.c Lines 799 to 802 in 647cb62
To
You should then get data event and break event |
Well, the uart.c I have is apparently older than the one you quote...so, I need to figure out how to update my local "/home/mac/.platformio/packages/framework-espidf/" I guess... That said, I have questions about your proposed change. I think I agree this will get a data and break events. But, what triggers the move of the FIFO contents to the ring buffer (which I think is where |
That if block evaluating to true |
Ok, so, not being able to figure out how to update my platformio 3.x ESP32-IDF, I show horned parts of the 4.x until I could compile. Then I modified uart.c with the suggestion above. It appears to trigger the fifo read. But, I never see an event.size less than 120. (I did try to read only 32 bytes at the break & first data event after, but that causes a panic.) |
I don't know if your changes caused any problems but what happens if you use the stock uart events example code? |
Hmm...didn't try it with any stock examples. I don't think I'm using anything but stock uart events...but, I may not understand. But, I did tinker some more with my code. And, the mod does appear to work... BUT I never see an event.size less than 120. I think, there is something going on that causes the reading of data to run past the 512 bytes. I see the input count go over 900, then back to 0 when a break is detected again. (So, I get to 480 when the break is detected and the next data event has event.size at 120. A read of that gets the last 32 bytes, plus 88. Haven't figured out to check if the 88 are junk or the beginning of the next 512...) But, with the current code I get correct values for byte 1 thru 512. And, get correct values as they change (that is, as the source changes the byte values of various channels.) I need to look at what I have currently and experiment some more. |
After more experimentation, I always get event.size = 120. |
For 1 packet you should get You should not read anything at the break event |
I added code to watch the even.size that is received when the event is triggered. It always returns event.size = 120. So here's how I think the low level driver works (Note I am now using pre release code. The new code has added a hal level and the mod mentioned above is not being used.):
|
Yes the mod is required because of 6 |
But, with the mod or without I get event.size = 120, never 32. And the first 32 bytes of that 120 are the expected last 32 of the DMX burst. |
Then the mod is not working. Show your code. |
As for the mod, the newer uart.c has changed so much I can't figure out where to put the mod now... uart.c at line 850 now is: My code is: https://github.com/macdroid53/NoArduino uart.c that I have is in src/uart.c.fyi I think the pertinent code is src/dmx.c The print statement at line 113 shows four 120 byte data events, then one break event with 120 bytes, then repeats. |
Yes it goes at 850. Break event doesn't have a size and shouldn't trigger a read bytes. |
I'm confused. Of course the break event doesn't have a size...The original thought was that 32 bytes were being left in the buffer when the break triggered and never read because the threshold was not reached and the FIFO was not transferred to the buffer. And the mod was to have a read event triggered when a break was detected, so that any orphan bytes in the FIFO, in this case 32 bytes, would be transferred from the FIFO to the buffer so as to loose them because of clearing that happens when a break is detected. |
Hi, @luksal `1 pack = 1 break + 1 MAB(mark after break) + 1 SC (start code) + 512 slots + 1 MTBP (mark time between packets) and the MTBP is within 0 ~ 1S, so if MTBP > rx_idle, the hardware will generate the rx_idle interrupt, and the application can get DATA event. The software can receive data packets according to the following process:
thanks !! |
In practice the MTBP is 2 stop bits (~8uS). Thus never triggering rx_idle (unless I don't understand...always possible) Also, I find no reference to rx_idle in uart.c and no event definition for it. |
I re-checked a few things. My logic analyzer capture shows that the SC byte is in the packet. So where is the SC going? |
Because you read too many bytes of the frame before. You should be keeping track independently of event.size. |
Ok, so I finally think I get what you've been saying. My apologies for having old, slow synapses. If we call the 513 bytes a packet and packets are received over time, we can number the packets Pn. Where "n" is the number of the latest packet. Since there can be data received (because of any latency source) after the break is detected and will be in the buffer. So the data event triggered by the break detect (with code modification as discussed) will include the remainder of packet Pn and the start code (and possibly more) of packet Pn+1. I've modified my test code up line of the modified uart.c and it does appear to be working. So, what needs to happen to get the modified uart.c code integrated as a fix? |
Either emptying the fifo on break has to become default behavior or there has to be a new config option to enable it. |
I would also agree with merging this change into esp-idf. As it stands, the break is essentially delivered asynchronously to the serial data, which doesn't make sense in many applications where detecting a break is required. This patch just makes sure the break is delivered in the correct place in the serial stream. Personally I don't see much reason why it shouldn't be the default behaviour. Perhaps someone could prepare a pull request, if those are accepted here? I can do it if required. -Alan |
@xiongyumail Would Espressif be willing to merge such a patch if it were provided? |
@acf-bwl Thanks for contribution, a Pull Request is welcome and appreciated. Thanks. |
I have been lurking and I have a question. I am about to code an app the requires receiving arbitrary length packets. There is a break sent immediately after the last byte in order to tell the receiver the packet has ended. Will the bug being described here make it impossible to implement this? Another question: If the break is received asynchronously and then the last bytes cannot be read, wouldn't that mean that data can be lost if a break happens shortly after receiving? This seems like a serious breach of generic uart standards. EDIT: Never mind. I'm changing the break to an idle in my protocol. I don't want to take any chances. |
Sorry for the delay. I've made a pull request #5959 with @negativekelvin 's above patch. Thanks, |
There is the other way of handling DMX512 packet using UART. I propose to use MARK before BREAK - MBB time (2Sec > MBB > 0) to detect end of packet without additional changes in UART driver. In most of DMX512 controllers this time is not zero and it allows to use existing hardware UART TOUT feature to detect the end of DMX512 packet. The start of DMX condition is still BREAK.
I tested my dmx512 receiver and transmitter over RS485 and this approach works just fine. |
@alisitsyn that is definitely better when the timeout can be used to detect end of frame and in that case you would not want the break to empty the buffer so if there is still a need to have that it should probably be configurable at runtime |
@negativekelvin, |
Hi all, I ran into the same problem and after a bit of searching I stumbled onto this thread. So with all this info I managed to fix the issue by evaluating the break condition first, then copy over the rx data from the fifo and pass the remainder length in the UART_BREAK event. ( at the lines specified by @negativekelvin) I'm not sure though if this could potentially impact the correct working of other serial protocols though, but for DMX512 it seems to work fine. ...
}
else if(uart_intr_status & UART_INTR_BRK_DET) {
uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), UART_INTR_BRK_DET);
rx_fifo_len = uart_hal_get_rxfifo_len(&(uart_context[uart_num].hal));
uart_hal_read_rxfifo(&(uart_context[uart_num].hal), p_uart->rx_data_buf, &rx_fifo_len);
uart_event.type = UART_BREAK;
uart_event.size = rx_fifo_len;
}
else if ((uart_intr_status & UART_INTR_RXFIFO_TOUT)
|| (uart_intr_status & UART_INTR_RXFIFO_FULL)
|| (uart_intr_status & UART_INTR_CMD_CHAR_DET)
) {
... This way, I can read the out when handling the case UART_BREAK:
if(udev->state == DMX_RX_DATA)
{
//read the remainder
uart_read_bytes(udev->uart.nr,
tmp, // do something interesting with these bytes
event.size,
portMAX_DELAY);
ESP_LOGI(TAG, "DATA (%d bytes)\n", event.size);
}
else
{
uart_flush_input(udev->uart.nr);
}
xQueueReset(udev->rx_queue);
udev->state = DMX_RX_BREAK;
break;
The 33 bytes left, is because I am planning to work on an RDM controller so I need the startcode to distinguish so I don't discard the first byte in the buffer. |
after a quick further read of the uart driver, I guess the select notification callback should also be called if in fact there was data in the rx fifo... So you'd end up with: ...
}
else if(uart_intr_status & UART_INTR_BRK_DET) {
uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), UART_INTR_BRK_DET);
rx_fifo_len = uart_hal_get_rxfifo_len(&(uart_context[uart_num].hal));
uart_hal_read_rxfifo(&(uart_context[uart_num].hal), p_uart->rx_data_buf, &rx_fifo_len);
uart_event.type = UART_BREAK;
uart_event.size = rx_fifo_len;
UART_ENTER_CRITICAL_ISR(&uart_selectlock);
if (rx_fifo_len && p_uart->uart_select_notif_callback) {
p_uart->uart_select_notif_callback(uart_num, UART_SELECT_READ_NOTIF, &HPTaskAwoken);
}
UART_EXIT_CRITICAL_ISR(&uart_selectlock);
}
else if ((uart_intr_status & UART_INTR_RXFIFO_TOUT)
|| (uart_intr_status & UART_INTR_RXFIFO_FULL)
|| (uart_intr_status & UART_INTR_CMD_CHAR_DET)
) {
... Anyway, I am curious as to what you guys think... |
Hi, I also found this situation in the ESP-IDF 4.1 version that triggered the event only after receiving 120 bytes or idle timeout, and then I used uart_set_rx_full_threshold and passed in the value of threshold 2 to solve this problem(If set to 1, there will be an error byte data of 0). |
A possible solution for this issue is very simple: read byte by byte from UART instead of reading 120 bytes when its FIFO is full. BREAK can happen any time.... just make sure that IDF driver always gets every single byte from UART and copy it to its internal RingBuffer. Doing so, when BREAK happens, the RingBuffer would be already complete and it will be possible to catch the BREAK event and then process the data already copied. In order to read byte by byte, just initialize the UART IDF Driver and then execute |
After some testing, I found out that ERROR event takes precedence over DATA UART event. Example: The IDF UART driver will raise these events in the Event Queue: 1- IDF setting FIFO Full to 20 bytes:
2- IDF setting FIFO Full to 120 bytes (IDF default setting):
Therefore, it will behave differently depending on how UART IDF RX IRQ is set. |
This issue is directly relevant to LIN bus. In LIN, each frame starts with a break. So I need to clear my rx buffer whenever a break is detected and then it fills-up until a number of bytes is received. Then I will know that the first byte is the Sync, then PID, etc... But the ESP-IDF sdk functions abstract this too much and I have to hack around these functions that are not ideal for this use case. I was planning to implement my own ISR to do this, but then I see that that custom ISR will be unsupported in the future?!: 473974c#diff-e69821beb8fd51e70825b947e7b520b90aae6cfc98241204cb6f30bcc79c4730 How should I synchronize my rx buffer to the break? i.e. every break should reset the buffer. FYI, here's my stupid code that works 90% of the time but sometimes gets out of sync: //// this is a terrible hack, please don't actually use this!
// returns length read
int LIN_driver_rx_header(uint8_t *pid, TickType_t ticksToWait)
{
uart_event_t event;
while (xQueueReceive(lin_uart_event_queue, (void *)&event, ticksToWait))
{
if (event.type == UART_BREAK)
{
//Event of UART RX break detected
// run slave task once per break detected
break;
}
}
if (event.type == UART_BREAK)
{
// Is there a LIN frame header received?
uint8_t slave_hdr_buf[4]; // we might recv some junk before the break (0) and sync (55)
int rx_got_len = 0;
int tries = 0; // prevent lockup
while (tries++ < 4 && rx_got_len < sizeof(slave_hdr_buf))
{
rx_got_len += uart_read_bytes(lin_uart_num, (uint8_t *)&slave_hdr_buf[rx_got_len], 1, ticksToWait);
if (rx_got_len >= 2) // need at least 2 bytes
{
ESP_LOG_BUFFER_HEX_LEVEL("LIN HDR", slave_hdr_buf, rx_got_len, ESP_LOG_VERBOSE);
if (rx_got_len == 2 && slave_hdr_buf[0] == 0x55) //check break (0) and sync (55) are in the correct spot
{
*pid = slave_hdr_buf[1]; // PID is last byte of header
return 1; // non-zero means we rec a header with PID
}
if (rx_got_len == 3 && slave_hdr_buf[1] == 0x55) //check break (0) and sync (55) are in the correct spot
{
*pid = slave_hdr_buf[2]; // PID is last byte of header
return 1; // non-zero means we rec a header with PID
}
if (rx_got_len == 4 && slave_hdr_buf[2] == 0x55) //check break (0) and sync (55) are in the correct spot
{
*pid = slave_hdr_buf[3]; // PID is last byte of header
return 1; // non-zero means we rec a header with PID
}
}
}
}
return 0; // 0 means we didn't get a valid header (i.e. timeout)
} the event queue was setup like this: |
In my use case I was trying to read the final UART logs from a device that was shutting down. The break event would fire before the timeout event and I would lose the final bytes from the logs. A simple workaround was to decrease the timeout from 10 to 2. This worked for my use case: uart_set_rx_timeout(UART_NUM_1, 2); #5959 worked for me also without this workaround, so it would be great to get that merged. |
I'm currently working on a DMX512 library for the ESP32 https://github.com/luksal/ESP32-DMX-RX .
DMX512 is a protocol based on RS485 where after a break signal 512 bytes are transmitted.
The main concept is running an infinite loop and call
xQueueReceive
periodically and then checking theevent.type
.When a
UART_DATA
is received, I'm copying the received data to an array and waiting for the next event. That way I'm getting 120 bytes each event, in total 480 until the break occurs.How can I receive the last 32 bytes? Because when
event.type
isUART_BREAK
, then callinguart_get_buffered_data_len
equals 0 and trying to useuart_read_bytes
fails at this point.The text was updated successfully, but these errors were encountered: