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

nrf5x: Fix assert test on SERIAL_RESERVED_CHAR_MATCH #6748

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
7 participants
@andrewleech
Contributor

andrewleech commented Apr 26, 2018

Description

I'm migrating an existing project to the recently-merged nrf52 / sdk 14.2 codebase in master.
I use a uart serial with match character for automatic depacketizing.

On current master I now get an assert fail as it's testing that char_match == SERIAL_RESERVED_CHAR_MATCH where I believe it should be the opposite; char_match should not be allowed to be SERIAL_RESERVED_CHAR_MATCH

This is with GCC and the debug.json build profile.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from marcuschangarm Apr 26, 2018

@@ -1697,7 +1697,7 @@ void serial_rx_asynch(serial_t *obj, void *rx, size_t rx_length, uint8_t rx_widt
MBED_ASSERT(obj);
MBED_ASSERT(rx_width == 8);
MBED_ASSERT(rx_length < 256);
MBED_ASSERT(char_match == SERIAL_RESERVED_CHAR_MATCH);
MBED_ASSERT(char_match != SERIAL_RESERVED_CHAR_MATCH);

This comment has been minimized.

@0xc0170

0xc0170 Apr 26, 2018

Member

I am not certain why it asserts even on the condition as it is ? As default value for char_match is this reserved character, thus if read without character match is used, then char_match contains reserved value (=not used) ?

This comment has been minimized.

@andrewleech

andrewleech Apr 26, 2018

Contributor

Ah good point, perhaps this assert needs to be just removed entirely... I haven't checked if anything similar exists in the previous nordic platforms.

This comment has been minimized.

@0xc0170

0xc0170 Apr 26, 2018

Member

I think so. I understand the rest of the parameters there to be checked (not supported length , width etc) but asserting on char match does not make sense to me. Please verify

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 26, 2018

Contributor

The assert is there to check that the default value (SERIAL_RESERVED_CHAR_MATCH) hasn't been changed.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 26, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 26, 2018

I use a uart serial with match character for automatic depacketizing.

The new serial driver doesn't support character matching since reception is handled by DMA which only stops once the buffer is full.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 26, 2018

That would explain that. No interrupt driver ? I would not expect to have DMA iwth character match thus if character match is selected, no dma involved ? Or that is also not possible thus that assert?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 26, 2018

Everything goes through the DMA now - it is the only way to prevent data loss it is the only API supported by the UARTE peripheral. The DMA buffers are copied to an atomic fifo which getc reads from. Interrupts are generated when needed.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Apr 26, 2018

Oh, the character match was a killer feature for me. :-(
Well I guess I'll change this PR to add a comment to the assert to alert users like me why that assert is there.

Assuming there's still an interrupt available on each char received (?) I suppose I can add the char match back in my app.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 26, 2018

Oh, the character match was a killer feature for me. :-(

Who do you think pushed for it initially? 😄 I didn't implement it since the DMA takes away the performance benefit and you already have the full buffer when you get the interrupt.

Assuming there's still an interrupt available on each char received (?) I suppose I can add the char match back in my app.

Yes, but the interrupt wont be per character but rather per full DMA buffer, which size you can configure.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Apr 27, 2018

Oh, this will be a really big problem for receiving variable sized messages.

My current project is porting from nrf51 to nrf52810, I'm working on filling out the 52810 support in mbed, it's a fantastic (cheap) chip.

It needs to be able to receive and handle messages between 2 character and 20 character long, with a null terminator on each one.
In fact most of my embedded projects have variable length usart messages where the length is either in the header of the message or a fixed end of message byte.

I've just been reading the uarte / easydma docs, I wasn't aware of it previously... I don't know how it's going to be possible to support variable length messaging... but yes I can see more the limitations and why you've implemented what you have!

@andrewleech andrewleech force-pushed the andrewleech:nrf52_serial_match_assert branch from b60c397 to 627d028 Apr 27, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 27, 2018

Ah, sorry, there is a timeout to flush the buffer so even if you only get 1 character you'll still get the interrupt so it should be straight forward to handle variable length messages.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Apr 27, 2018

Oh really, I didn't see any reference to a timeout (haven't looked deep/close enough clearly).
Currently the sending device often has multiple messages back to back (which has caused missed messages on existing hardware) so my options are to make the other micro have fixed length or a reasonable timeout delay between each, either should be possible...
Thanks for the info!

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Apr 27, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 27, 2018

Build : SUCCESS

Build number : 1867
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6748/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 27, 2018

#6748 (comment)

@LMESTM The above failure, can you reproduce locally? I noticed it in the last days in some CI runs

/morph test
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 27, 2018

Currently the sending device often has multiple messages back to back (which has caused missed messages on existing hardware) so my options are to make the other micro have fixed length or a reasonable timeout delay between each, either should be possible...

I wouldn't do anything with the other micro just yet. There is a very generous (and configurable) atomic fifo between getc and the DMA buffers.

Once you get an interrupt, it is either because the DMA buffer is full or the other micro is done sending. Just read out all characters in the fifo whenever you get an interrupt and you shouldn't miss any data.

@cmonr cmonr merged commit e1cc455 into ARMmbed:master Apr 30, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9173 cycles (-1031 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10112B
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment