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.

Sign up for GitHub

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

Jump to bottom

Fix unpacked behaviour of crc32_bb fixes #1407 #1504

Merged

marcusmueller merged 5 commits into gnuradio:master from noc0lour:fix_crc32_unpacked_behaviour

Feb 8, 2018

Member

noc0lour commented Oct 31, 2017 •

edited

This requires repacking unpacked binary data (currently only LSB) and calculating the CRC.
Previous behaviour generated a mixed packed/unpacked stream of bytes.

I added a new test for the correct behaviour and fixed a test relying on the wrong/old behaviour.
fixes #1407

marcusmueller requested changes

View reviewed changes

Member

marcusmueller left a comment

Full in favor if fixing the "C++ VLA" issue, either by use of std::vector, or a preallocated buffer, or by replacing the "packed CRC" with an "unpacked" one.

gr-digital/lib/crc32_bb_impl.cc

+                    unsigned int crc = 0;
+                    d_crc_impl.reset();
+                    if (!d_packed){
+                      const size_t n_packed_length = 1 + ((packet_length - 1) / 8);

Member

marcusmueller Nov 1, 2017

Are we 100.000001% sure that packet_length != 0 at this point?

Member Author

noc0lour Nov 1, 2017 •

edited

Hm, we could test that and throw? or use some less efficient algorithm? From below: if packet_length==crc_length && d_check this might actually do harm, but is this realistic with TSBs?

Member Author

noc0lour Feb 5, 2018

Why is packet_length != 0 an issue here? If I add -0 to 1 it will remain 1. Right now I cannot recall why I'm doing this calculation here, but I'm pretty sure it has some reason...

gr-digital/lib/crc32_bb_impl.cc Outdated

                       d_crc_impl.process_bytes(in, packet_length - d_crc_length);
-                      crc = d_crc_impl();
+                      crc = calculate_crc32(in, packet_length - d_crc_length);

Member

marcusmueller Nov 1, 2017

looks like if ninput_items[0] == d_crc_length, you call calculate_crc32 with an input argument of packet_length = 0

Member Author

noc0lour Feb 5, 2018

Will this ever happen for a valid crc32 packet? If yes will it explode?

gr-digital/lib/crc32_bb_impl.cc Outdated

+                    d_crc_impl.reset();
+                    if (!d_packed){
+                      const size_t n_packed_length = 1 + ((packet_length - 1) / 8);
+                      unsigned char packed_buffer[n_packed_length];

Member

marcusmueller Nov 1, 2017 •

edited

The gods of C99 do smile upon you, while C++'s olymp frowns and shakes its heads:

This isn't valid C++. You'll need to new the hell out of this buffer, and free it at the end, or use a std::vector. Both would also inherently memset it to 0.

Oh, by the way, don't do that. Memory allocation is expensive, and you really don't need to reallocate every time work is called. Just give the class a member buffer of some fixed length (a page?), allocate that in the ctor, deallocate in dtor, and call d_crc_impl.process_bytes() repeatedly.

If you're in a hurry, go the std::vector route. It's easy, and that's why we do it all over the place.

Member Author

noc0lour Nov 1, 2017

hm, yeah we could limit the unpacked operation to multiple of 8 items. then the call to process bytes on a single byte might work.

gr-digital/lib/crc32_bb_impl.cc Outdated

+                      unsigned char packed_buffer[n_packed_length];
+                      memset(packed_buffer, 0, n_packed_length);
+                      for (size_t bit = 0; bit < packet_length; bit++){
+                        packed_buffer[bit/8] |= (in[bit] << (bit % 8));

Member

marcusmueller Nov 1, 2017

We should really have a VOLK kernel for this. should. Feels like I thought we had a "packer" kernel; @n-west ?

Nothing wrong with this code, though! 😄

Thinking about that, we should probably just use a different CRC polynomial (this polynomial g(x) "interpolated" by doing g⁸(x), maybe?) and not pack before CRC. Anyway, that's math for another day. (Which doesn't mean I wouldn't encourage you to do that math or just try. Just saying my own brain's too mushy right now.)

Member

marcusmueller Nov 1, 2017 •

edited

Ah jetzt ja. (eine Insel.)

So, indeed,

    boost::crc_optimal<32,
                       boost::crc_32_type::truncated_polynominal * boost::crc_32_type::truncated_polynominal *
                           boost::crc_32_type::truncated_polynominal * boost::crc_32_type::truncated_polynominal *
                           boost::crc_32_type::truncated_polynominal * boost::crc_32_type::truncated_polynominal *
                           boost::crc_32_type::truncated_polynominal * boost::crc_32_type::truncated_polynominal,
                       0xFFFFFFFF, 0xFFFFFFFF, true, true>
        padded_crc;

at least in my stupid test case works directly on unpacked data and delivers the same output as crc_32_type (which is what the crc32_impl uses) on packed data.

Why the ugly unrolled potentiation? The polynomial is a template argument, i.e. needs to be a compile-time constant. Prior to C++11, there's not constexpr to let me define a compile-time evaluated function, and prior to C++14, you couldn't use a for loop in that function.

gr-digital/lib/crc32_bb_impl.cc

                   int
                   crc32_bb_impl::work(int noutput_items,
                                       gr_vector_int &ninput_items,
                                       gr_vector_const_void_star &input_items,
                                       gr_vector_void_star &output_items) {
                     const unsigned char *in = (const unsigned char *) input_items[0];
                     unsigned char *out = (unsigned char *) output_items[0];
-                    long packet_length = ninput_items[0];
+                    size_t packet_length = ninput_items[0];

Member

marcusmueller Nov 1, 2017

👍 replacing signed with unsigned integer types where right is definitely a good idea wherever one encounters an opportunity to do so. Reduces potential for bugs that the compiler would tell you about.

gr-digital/python/digital/qa_crc32_bb.py Outdated

+                          unpack2,
+                          crc_unpacked,
+                          sink2
+                      )

Member

marcusmueller Nov 1, 2017

pedantic, but: indents not identical in the two connect calls.

Member Author

noc0lour Nov 1, 2017

shouldn't copy stuff from other places with random indentation.

Member Author

noc0lour commented Nov 1, 2017

Alternatively we could mandate packed operation and remove the alternative to operate on unpacked data.

Member

marcusmueller commented Nov 1, 2017

I'd really rather use a CRC that works on unpacked than that :)

Member

marcusmueller commented Nov 1, 2017

wait, are we not setting an io multiple of 8 in unpacked mode?

Member Author

noc0lour commented Nov 1, 2017

Yes we do, this is TSB mode, so we can actually rely on that?

Member

marcusmueller commented Nov 1, 2017

damned if I knew what we can trust in TSB mode. So yeah, in TSB mode we can't simply let the scheduler guarantee that we get multiples of 8, since that is incompatible with the TSB contract. Sooooooo yeah, we need the ability to deal with a 35 bit unpacked packet. Can you try whether #1504 (comment) works without the introduction of the "packed" 1/8-sized buffer?

Member Author

noc0lour commented Nov 1, 2017

I'll put that into a unit test, I guess, and see if this fails or not.

Member

marcusmueller commented Nov 1, 2017

You're a personal hero of mine :) at least for today

Member

marcusmueller commented Feb 3, 2018

Hey! You promised us a unit test, and I pinky promise a speedy merge if you do that (I wouldn't know who I'd rather ask for testing than my personal CI hero, @noc0lour)

Member Author

noc0lour commented Feb 3, 2018

Hm, I forgot this PR even exists... Now I have to figure out what I did there

noc0lour force-pushed the fix_crc32_unpacked_behaviour branch from 1591484 to 4de6bdc Compare

February 3, 2018 23:09

marcusmueller added this to the Release 3.7.12 milestone

marcusmueller approved these changes

View reviewed changes

noc0lour added 5 commits

February 7, 2018 18:32


          digital: handle unpacked bytes correctly in CRC32_bb

cd2e78a


          crc32: use one std::vector buffer for unpacked & packed operation

fdc09e6


          crc32: add test with length 35

2604bf5


          digital: check length in crc32, remove unused variable


          digital: fix PEP8 in qa_crc32_bb

9e8a4ad

noc0lour force-pushed the fix_crc32_unpacked_behaviour branch from a518ec4 to 9e8a4ad Compare

February 7, 2018 17:32

marcusmueller merged commit b6caf24 into gnuradio:master

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