search

Avnu / OpenAvnu

OpenAvnu - an Avnu sponsored repository for Time Sensitive Network (TSN and AVB) technology
472 stars 292 forks source link

MRPD - can't emit vectors across multiple PDUs #83

Open andrew-elder opened 10 years ago

andrew-elder commented 10 years ago

Current code to emit a sequence of vectors does not gracefully handle the case where the message buffer becomes full.

davidcemin commented 10 years ago

Andrew, Judging by your description, i assume you saw this bug on the code by inspection. Could you please point where is that on the code so whoever will fix it have a starting point. Also, it would be good to have a way of validating that the problem was solved, i.e., after fixing, how can we make sure it is ok.

thanks!

andrew-elder commented 10 years ago

This issue is in msrp.c function msrp_txpdu() and msrp_emit_talkervectors(), msrp_emit_listenvectors() and msrp_emit_domainvectors(). Say there are enough talker vectors to overfill the PDU. There is no mechanism to send the rest of the vectors in another PDU. The same handling also has to be present for the listener and domain vectors.

We will need to write a test application specifically for testing this issue.

craiggunther commented 10 years ago

Sounds like a great use of CppUTest unit testing :-)

When responding to a txLA/txLAF be sure when fixing this that the LeaveAll Event is only set in the first PDU. If it gets set in the second PDU it will cause all attributes in the first PDU to eventually be removed.

andrew-elder commented 10 years ago

I was thinking the exact same thing!

On 10/13/2014 11:28 AM, Craig Gunther wrote:

Sounds like a great use of CppUTest unit testing :-)

When responding to a txLA/txLAF be sure when fixing this that the LeaveAll Event is only set in the first PDU. If it gets set in the second PDU it will cause all attributes in the first PDU to eventually be removed.

— Reply to this email directly or view it on GitHub https://github.com/AVnu/Open-AVB/issues/83#issuecomment-58908771.

Andrew Elder AudioScience, Inc. +1-585-271-8870 www.audioscience.com

andrew-elder commented 10 years ago

I'd like to add cpputest as a submodule to Open-AVB. I'd like to make it obvious to the user that cpputest is an external module (and may be one of many eventally), so I'd propose we create at the project root /thirdparty/cpputest Comments....

craiggunther commented 10 years ago

That would be fantastic!  The fun part is going to be getting it integrated into the Windows and Linux build environments.  This is something we want to automatically run every time someone types "make".  I have been wanting to do this for a long time, but time has just been impossible to find :-( Unit testing in connection with the automated build system that Levi has been working on is going to be huge! Once this is going I will certainly be able to add a bunch of unit tests that will exercise SRP.  We may find that a bit of rework is going to be necessary to make the code more able to be unit tested, but that will be a worthwhile exercise.

I'd like to add cpputest as a submodule to Open-AVB. I'd like to make it obvious to the user that cpputest is an external module (and may be one of many eventally), so I'd propose we create at the project root /thirdparty/cpputest Comments....— Reply to this email directly or view it on GitHub.

andrew-elder commented 10 years ago

I would expect we would use cmake for windows and linux build environments.

On 10/14/2014 10:34 AM, Craig Gunther wrote:

That would be fantastic! The fun part is going to be getting it integrated into the Windows and Linux build environments. This is something we want to automatically run every time someone types "make". I have been wanting to do this for a long time, but time has just been impossible to find :-( Unit testing in connection with the automated build system that Levi has been working on is going to be huge! Once this is going I will certainly be able to add a bunch of unit tests that will exercise SRP. We may find that a bit of rework is going to be necessary to make the code more able to be unit tested, but that will be a worthwhile exercise.

  • Craig From: andrew-elder notifications@github.com To: AVnu/Open-AVB Open-AVB@noreply.github.com Cc: Craig Gunther craiggunther@yahoo.com Sent: Tuesday, October 14, 2014 4:29 AM Subject: Re: [Open-AVB] MRPD - can't emit vectors across multiple PDUs (#83)

I'd like to add cpputest as a submodule to Open-AVB. I'd like to make it obvious to the user that cpputest is an external module (and may be one of many eventally), so I'd propose we create at the project root /thirdparty/cpputest Comments....— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/AVnu/Open-AVB/issues/83#issuecomment-59054803.

Andrew Elder AudioScience, Inc. +1-585-271-8870 www.audioscience.com

jdkoftinoff commented 10 years ago

Also see CMake's CTest functionality to automate execution of the unit tests:

http://www.cmake.org/cmake/help/v2.8.8/ctest.html

--jeffk++

On Oct 14, 2014, at 12:52, andrew-elder notifications@github.com wrote:

I would expect we would use cmake for windows and linux build environments.

On 10/14/2014 10:34 AM, Craig Gunther wrote:

That would be fantastic! The fun part is going to be getting it integrated into the Windows and Linux build environments. This is something we want to automatically run every time someone types "make". I have been wanting to do this for a long time, but time has just been impossible to find :-( Unit testing in connection with the automated build system that Levi has been working on is going to be huge! Once this is going I will certainly be able to add a bunch of unit tests that will exercise SRP. We may find that a bit of rework is going to be necessary to make the code more able to be unit tested, but that will be a worthwhile exercise.

  • Craig From: andrew-elder notifications@github.com To: AVnu/Open-AVB Open-AVB@noreply.github.com Cc: Craig Gunther craiggunther@yahoo.com Sent: Tuesday, October 14, 2014 4:29 AM Subject: Re: [Open-AVB] MRPD - can't emit vectors across multiple PDUs (#83)

I'd like to add cpputest as a submodule to Open-AVB. I'd like to make it obvious to the user that cpputest is an external module (and may be one of many eventally), so I'd propose we create at the project root /thirdparty/cpputest Comments....— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/AVnu/Open-AVB/issues/83#issuecomment-59054803.

Andrew Elder AudioScience, Inc. +1-585-271-8870 www.audioscience.com — Reply to this email directly or view it on GitHub.

andrew-elder commented 10 years ago

Have made progress getting set up to test this. I can observe the failure in CppUTest fairly easily. Along the way, I have observed 2 other bugs in msrp.c and potentially other modules as well. I need to create a couple more issues for them.

jdkoftinoff commented 10 years ago

Hi Andrew and others

Please watch this presentation on the clang sanitize tools. I HIGHLY recommend that you run the unit tests under clang-sanitize (or gcc 4.9 sanitize) tools. It is not just for C++ code either.

https://www.youtube.com/watch?v=V2_80g0eOMc

Jeff

On Oct 22, 2014, at 07:38, andrew-elder notifications@github.com wrote:

Have made progress getting set up to test this. I can observe the failure in CppUTest fairly easily. Along the way, I have observed 2 other bugs in msrp.c and potentially other modules as well. I need to create a couple more issues for them.

— Reply to this email directly or view it on GitHub https://github.com/AVnu/Open-AVB/issues/83#issuecomment-60071598.

craiggunther commented 10 years ago

Thanks Jeff, this was excellent.  I wasn't sure I was willing to commit an hour's worth of time to watch this, but I found it so interesting that I didn't realize the hour had passed. I agree, this looks like another fantastic tool. From: Jeff Koftinoff notifications@github.com To: AVnu/Open-AVB Open-AVB@noreply.github.com Cc: Craig Gunther craiggunther@yahoo.com Sent: Wednesday, October 22, 2014 5:42 AM Subject: Re: [Open-AVB] MRPD - can't emit vectors across multiple PDUs (#83)

Hi Andrew and others

Please watch this presentation on the clang sanitize tools. I HIGHLY recommend that you run the unit tests under clang-sanitize (or gcc 4.9 sanitize) tools. It is not just for C++ code either.

https://www.youtube.com/watch?v=V2_80g0eOMc

Jeff

On Oct 22, 2014, at 07:38, andrew-elder notifications@github.com wrote:

Have made progress getting set up to test this. I can observe the failure in CppUTest fairly easily. Along the way, I have observed 2 other bugs in msrp.c and potentially other modules as well. I need to create a couple more issues for them.

— Reply to this email directly or view it on GitHub https://github.com/AVnu/Open-AVB/issues/83#issuecomment-60071598. — Reply to this email directly or view it on GitHub.

SiMet commented 3 years ago

Is there any update in this topic? :) We faced this issue and see that we are not alone