Skip to content
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

Adding field for HW-provided sample count. #98

Merged
merged 2 commits into from Nov 9, 2018
Merged

Conversation

bhilburn
Copy link
Contributor

@bhilburn bhilburn commented Oct 1, 2018

This allows you to track things like dropped packets and data overruns
in the SigMF recording.

Per our discussion at GRCon. Please review @pwicks86 and/or @storborg?

This allows you to track things like dropped packets and data overruns
in the SigMF recording.
@smunaut
Copy link
Contributor

smunaut commented Oct 1, 2018

?!? Why is 'length' not good enough ?
Just have a segment 0-1000 then the next one 1500-xxx, I don't see why this is necessary at all.

@smunaut
Copy link
Contributor

smunaut commented Oct 1, 2018

Arf, just saw the other PR removing 'length' so you're essentially renaming a field ... then I'd suggest 'sample_count' instead of 'sample_number' since that's the nomenclature used already at other places of the spec.

@bhilburn
Copy link
Contributor Author

bhilburn commented Oct 1, 2018

@smunaut - So the length field was actually not meant for that purpose - see #97 for more, there. It was originally added to describe the number of samples in a capture segment, which we have now decided is not appropriate as it's redundant information and makes it possible to create incorrect recordings (what happens if length disagrees with the number of samples?)

You're right, we could simply re-purpose the length field, though - which is honestly not something I considered. That said, I think the naming in this PR is more intuitive than calling it length. Happy to hear differing opinions, though.

@bhilburn
Copy link
Contributor Author

bhilburn commented Oct 1, 2018 via email

@smunaut
Copy link
Contributor

smunaut commented Oct 1, 2018

Arf ok ... I was confused because the very title of this PR is "Adding field for HW-provided sample count.".

'number' still feels a bit weird. 'index' ? 'timestamp' ?
But yeah, just whatever people think, at this point it's just a name and there seems to be no existing name in the spec that would describe the same concept. (Obviously any future ones would have to stick to whatever is picked here)

@storborg
Copy link
Contributor

storborg commented Oct 3, 2018

To confirm, is the idea that this can be any monotonic sample counter, and doesn't have to start at zero?

@bhilburn
Copy link
Contributor Author

@smunaut - Yeah, sorry, title of the PR wasn't great. I'm worried index is too easily confused with our use of index elsewhere in the spec. That said...

@storborg - Yes, it can be any monotonic sample counter.

@smunaut @storborg - Perhaps the better name is sample_count, then?

@smunaut
Copy link
Contributor

smunaut commented Oct 10, 2018

Huh no ... _count is a terrible name because we already used "count" elsewhere ( in annotation IIRC ) to note the length of the annotation. (so it's a length and not index).

Where else in the spec is 'index' used ? or hwindex ?

@bhilburn
Copy link
Contributor Author

@smunaut - Argh, you're right. That's actually why I didn't use count to begin with, I had just forgotten.

If you search through the spec, you'll see that we use index heavily to refer to the index of a samples in the dataset file (e.g., sample_start in captures).

Something like hwindex is an interesting idea, though, and is perhaps more intuitive as to what the intention is. In the overrun event I describe in the changeset, for example, the hwindex of 1500 maps to the local index of 1000, due to the lost samples.

Thoughts? Is using hw too restrictive - are there other mechanisms by which this might happen? Can't think of any off the top of my head.

@bhilburn bhilburn self-assigned this Oct 10, 2018
@bhilburn
Copy link
Contributor Author

bhilburn commented Nov 9, 2018

Okay, we discussed this here at DeepSig, and I'm going to change it to core:global_index. I agree with the comments above that sample_number is a little too confusing. hwindex is better, but too narrow. I also considered absolute_index, but decided that was too confusing given that the only absolute reference to anything from the application's perspective is the sample index, so saying there is something else that's "absolute" is problematic.

I think global_index captures the fact that the overall system (including the HW, for example) has a index counter that might be different from the application's, and this can cover more complex systems like a radio multicasting sample streams to different processors.

@bhilburn bhilburn merged commit f513206 into master Nov 9, 2018
@jacobagilbert jacobagilbert deleted the dropped-data branch June 2, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants