Bug #530

Connect tagged stream blocks and UHD sink

Added by Martin Braun over 1 year ago. Updated 5 months ago.

Status:ClosedStart date:04/05/2013
Priority:NormalDue date:
Assignee:Sean Nowlan% Done:

0%

Category:gr-uhd
Target version:release-3.7.4
Resolution:fixed

Description

The tagged stream blocks use PDU length tags; UHD uses tx_sob and tx_eob.

We need to connect these. Two possibilities come to mind:

- Adapt usrp_sink to understand length tags
- Make a burst tagger block that adds tx_?ob tags which is connected before uhd.usrp_sink

uhd_usrp_sink_changes.patch Magnifier - patch (9.3 KB) Sean Nowlan, 05/03/2013 10:19 pm

test_uhd.py Magnifier - test file (3.64 KB) Sean Nowlan, 05/03/2013 10:19 pm

uhd_usrp_sink_changes2.patch Magnifier (7.91 KB) Sean Nowlan, 05/04/2013 03:18 am

uhd_usrp_sink_changes.patch Magnifier (8.3 KB) Sean Nowlan, 05/12/2013 03:31 am

uhd_usrp_sink_tagged_stream_support_gr3.7.patch Magnifier (15.2 KB) Sean Nowlan, 06/13/2013 02:26 am

uhd_tagged_stream_support.bundle (3.91 KB) Sean Nowlan, 06/13/2013 01:03 pm

History

#1 Updated by Josh Blum over 1 year ago

in other words:
Using a PDU blob to represent a burst rather than stream items with tags?

#2 Updated by Martin Braun over 1 year ago

No, I meant a streamed item of tags, just not sob/eob but rather 1 length tag as in every gr_tagged_stream_block.

This would be helpful for attaching the new ofdm blocks to uhd.

#3 Updated by Johnathan Corgan over 1 year ago

  • Status changed from New to Assigned
  • Assignee changed from Josh Blum to Johnathan Corgan
  • Target version set to release-3.6.5

#4 Updated by Sean Nowlan over 1 year ago

I'd like to work on this if there are no objections. I have to do it anyway for some of my own applications. I have a fairly good idea of how I'd do it within gr_uhd_usrp_sink:

- whenever a length tag is seen
- set a d_pdu_left_to_consume variable (or similar)
- set UHD meta data to indicate start-of-burst
- for every call to work(), decrement d_pdu_left_to_consume by number of samples sent
- when d_pdu_left_to_consume reaches zero, send end-of-burst mini packet to UHD

I would keep backwards compatibility with the tx_sob/tx_eob format, though I would set some guard to prevent the two models from interfering, i.e., stick with whichever tag I see first. I would also need to keep the tx_time tag. Thoughts?

#5 Updated by Martin Braun over 1 year ago

I think this is the correct way to do things.
Perhaps someone from Ettus (probably Josh) should be involved pre-merge to make sure it doesn't accidentally cause troubles w/ their hardware.

#6 Updated by Johnathan Corgan over 1 year ago

Sean--please implement this the way you describe and give us a patch or git branch. It will need some extensive testing, and we won't merge it until Ettus has a chance to review and comment.

However, to avoid confusion, please don't use the term PDU in any of the new code, this is an interface to a tagged stream block, and should probably use 'tsb' for its naming. A PDU interface to a USRP sink (by receiving it as an async message) might also be a desirable feature, but is a different beast altogether.

#7 Updated by Sean Nowlan over 1 year ago

Ok, understood. I'll call it "tsb" or length-tagged stream.

The async msg feature sounds interesting, but after I finish this, I believe it could be achieved through pdu_to_tagged_stream block --> tsb-capable USRP sink. Perhaps there are merits to giving the USRP sink its own async message interface; I haven't thought them through.

I'll probably just send you a patch. The timing will be in the next 1-1.5 weeks.

#8 Updated by Sean Nowlan over 1 year ago

I made these changes on top of master, current as of today (8fda7acad4f71645bded77b5a4806b21aba46078). I used the attached Python script to test the changes, but I'm sure I haven't tested every possible corner case. Perhaps I can enlist some help testing/vetting this code?

I did confirm this works for bursts down to about 50 microseconds on a Tektronix RSA6100. Getting secondary confirmation would be helpful; hopefully the good people at Ettus Research can help. :)

#9 Updated by Johnathan Corgan over 1 year ago

Sean--thanks for doing this. I've only glanced at the patch briefly, but it appears that it may be adding extra docs/functions instead of modifying them. Can you verify the patch is correctly formed?

I'm on the road, so I can't actually apply and test this for a few days, but Martin or Tom might.

#10 Updated by Sean Nowlan over 1 year ago

I added new public factory function that takes a len_tag_key argument, similar to the OFDM blocks. The original ones are still there. The sink needs to know what length tags to look for since it should not be hard-coded and it shouldn't just assume any tag it doesn't already understand is a length tag.

#11 Updated by Johnathan Corgan over 1 year ago

It would be preferable to add the length tag key as a parameter to the existing factory function, and give it a default argument so that existing code sees no change in behavior. This would avoid all the duplication.

#12 Updated by Sean Nowlan over 1 year ago

Ok, that makes sense. I've attached a new patch with the factory function changes. It replaces the last patch file.

#13 Updated by Sean Nowlan over 1 year ago

I discovered an issue with the previous patch. During long bursts that fill up the USRP buffer, the USRP was ACKing that zero samples were transmitted. I think this is expected behavior, but I was decrementing _nitems_remaining by ninput_items instead of num_sent, so bursts weren't lining up correctly. I've attached a patch that fixes this issue (should apply cleanly to freshly cloned master). I also changed _len_tag_key == "" and _len_tag_key != "" checks to _len_tag_key.empty() and not _len_tag_key.empty(), respectively.

#14 Updated by Johnathan Corgan over 1 year ago

  • Target version changed from release-3.6.5 to release-3.7.0

This did not make the 3.6.5 release window. Please submit a fresh patch against the new master branch. Thanks.

#15 Updated by Sean Nowlan over 1 year ago

Fixed to work on current master (e7ea85fb752c330de2f29ca5e666a0727be0aa51). I also added length tag generation to tags_demo in gr-uhd/examples/c++. I tested with many combinations of short and long bursts, sample rates, etc. I also included a test of gracefully canceling a burst if a new length tag shows up before the end of the previous burst (must be enabled in tag_source_demo.h). The smallest interval I checked was 5 microseconds on, 245 microseconds off at 10 MSps on an N200 (50 samples). Using a smaller sample rate for these durations caused late packets, but the standard tx_sob/tx_eob approach also caused late packets on my system. The largest burst length I checked was 950 ms on, 50 ms off at 5 MSps (4.75e6 samples).

#16 Updated by Sean Nowlan over 1 year ago

You could also use this bundle, which has a branch called uhd_tagged_stream_support

#17 Updated by Johnathan Corgan over 1 year ago

  • Target version changed from release-3.7.0 to release-3.7.1

This has been tested and is ready for merge; we are just waiting for the copyright assignment to complete.

#18 Updated by Johnathan Corgan about 1 year ago

  • Target version changed from release-3.7.1 to release-3.7.2

#19 Updated by Johnathan Corgan 11 months ago

  • Assignee changed from Johnathan Corgan to Sean Nowlan
  • Target version changed from release-3.7.2 to release-3.7.3

#20 Updated by Ben Hilburn 9 months ago

Is this bug still active or has it been merged?

#21 Updated by Martin Braun 9 months ago

Still active. There are some non-technical issues with the merging of this.

#22 Updated by Martin Braun 9 months ago

  • Assignee changed from Sean Nowlan to Nick Foster

#23 Updated by Sean Nowlan 7 months ago

I went ahead and reimplemented this on my own. Personal copyright assignment form was filed w/ FSF over a week ago. I'm not sure if I need to wait to hear back from them.

Here's the branch implementing this feature:

https://github.com/nowls/gnuradio/tree/usrp_sink_tagged_stream_support

#24 Updated by Martin Braun 7 months ago

  • Assignee changed from Nick Foster to Sean Nowlan

I'd like to give this a quick review before anyone merges anything.

#25 Updated by Martin Braun 7 months ago

Looks OK.

#26 Updated by Sean Nowlan 7 months ago

Martin suggested saving the length_tag as a PMT symbol instead of a string. I think this is a good suggestion. I end up checking the length of the length_tag string a handful of times in order to keep the tx_sob/tx_eob implementation mutually exclusive from the length_tag approach. Maybe if the length_tag argument string is empty, I should save the key as a pmt::PMT_NIL type. Then all my length_tag.empty() calls could be pmt::is_null(key) calls, which seems cleaner and more readable. I'm going to update this now on my github branch.

Unrelated to the above, I also thought of this: if tag propagation isn't guaranteed to be exact, there could be an off-by-one error. To give a specific example: if I'm using a block like a pfb_arb_resampler with a noninteger relative rate, sometimes it produces 1 more or less than the relative rate implies. Due to tag propagation issues, this could end up stranding samples between packet_len tagged streams in a packet flow. Any thoughts on how to handle this? My current implementation expects all samples to be accounted for in the packet_len values.

#27 Updated by Martin Braun 7 months ago

My gut feeling is that length tags should always be exact (i.e. make this a contract). Is there a way to fix this in the pfb_resampler?

#28 Updated by Sean Nowlan 7 months ago

Personally I think pfb_resampler should handle its own tag propagation. Currently it relies on block_executor's implementation, which is to use the relative rate. Tags drift over time, and they shouldn't be. I started issue #652 and provided a fix that stops tag offsets from drifting, but it still wouldn't preserve correct tagged stream boundaries.

#29 Updated by Johnathan Corgan 7 months ago

  • Target version deleted (release-3.7.3)

Agree that tagged streams must be exact; no need to change this submission to accommodate otherwise.

#30 Updated by Sean Nowlan 7 months ago

My github branch has the update to use a PMT instead of string for the member variable. https://github.com/nowls/gnuradio/tree/usrp_sink_tagged_stream_support

#31 Updated by Johnathan Corgan 5 months ago

  • Status changed from Assigned to Resolved
  • Target version set to release-3.7.4
  • Resolution set to fixed

#32 Updated by Johnathan Corgan 5 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF