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.

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
merged 5 commits into from Feb 8, 2018
Merged

Fix unpacked behaviour of crc32_bb fixes #1407 #1504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd2e78a
digital: handle unpacked bytes correctly in CRC32_bb
noc0lour Oct 31, 2017
fdc09e6
crc32: use one std::vector buffer for unpacked & packed operation
noc0lour Feb 3, 2018
2604bf5
crc32: add test with length 35
noc0lour Feb 3, 2018
6979643
digital: check length in crc32, remove unused variable
noc0lour Feb 7, 2018
9e8a4ad
digital: fix PEP8 in qa_crc32_bb
noc0lour Feb 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 34 additions & 17 deletions gr-digital/lib/crc32_bb_impl.cc
View file
Open in desktop
Expand Up @@ -40,20 +40,18 @@ namespace gr {
io_signature::make(1, 1, sizeof(char)),
io_signature::make(1, 1, sizeof(char)),
lengthtagname),
d_check(check), d_packed(packed),
d_npass(0), d_nfail(0) {
d_check(check), d_packed(packed){
d_crc_length = 4;
if (!d_packed) {
d_crc_length = 32;
d_unpacked_crc = new char[d_crc_length];
d_buffer = std::vector<char>(d_crc_length);
}else{
d_buffer = std::vector<char>(4096);
}
set_tag_propagation_policy(TPP_DONT);
}

crc32_bb_impl::~crc32_bb_impl() {
if (!d_packed){
delete[] d_unpacked_crc;
}
}

int
Expand All @@ -65,50 +63,69 @@ namespace gr {
}
}

unsigned int
crc32_bb_impl::calculate_crc32(const unsigned char* in, size_t packet_length){
unsigned int crc = 0;
d_crc_impl.reset();
if (!d_packed){
const size_t n_packed_length = 1 + ((packet_length - 1) / 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@noc0lour noc0lour Nov 1, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

if (n_packed_length > d_buffer.size()){
d_buffer.resize(n_packed_length);
}
d_buffer.clear();
for (size_t bit = 0; bit < packet_length; bit++){
d_buffer[bit/8] |= (in[bit] << (bit % 8));
}
d_crc_impl.process_bytes(&d_buffer[0], n_packed_length);
crc = d_crc_impl();
} else{
d_crc_impl.process_bytes(in, packet_length);
crc = d_crc_impl();
}
return crc;
}

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];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.

int packet_size_diff = d_check ? -d_crc_length : d_crc_length;
unsigned int crc;

if (d_check) {
d_crc_impl.reset();
if (packet_length <= d_crc_length){
return 0;
}
d_crc_impl.process_bytes(in, packet_length - d_crc_length);
crc = d_crc_impl();
crc = calculate_crc32(in, packet_length - d_crc_length);
if (d_packed) {
if (crc != *(unsigned int *) (in + packet_length - d_crc_length)) { // Drop package
d_nfail++;
return 0;
}
}
else{
for(int i=0; i < d_crc_length; i++){
if(((crc >> i) & 0x1) != *(in + packet_length - d_crc_length + i)) { // Drop package
d_nfail++;
return 0;
}
}
}
d_npass++;
memcpy((void *) out, (const void *) in, packet_length - d_crc_length);
} else {
d_crc_impl.reset();
d_crc_impl.process_bytes(in, packet_length);
crc = d_crc_impl();
crc = calculate_crc32(in, packet_length);
memcpy((void *) out, (const void *) in, packet_length);
if (d_packed) {
memcpy((void *) (out + packet_length), &crc, d_crc_length); // FIXME big-endian/little-endian, this might be wrong
}
else {
for (int i = 0; i < d_crc_length; i++) { // unpack CRC and store in buffer
d_unpacked_crc[i] = (crc >> i) & 0x1;
d_buffer[i] = (crc >> i) & 0x1;
}
memcpy((void *) (out + packet_length), (void *) d_unpacked_crc, d_crc_length);
memcpy((void *) (out + packet_length), (void *) &d_buffer[0], d_crc_length);
}
}

Expand Down
5 changes: 2 additions & 3 deletions gr-digital/lib/crc32_bb_impl.h
View file
Open in desktop
Expand Up @@ -36,7 +36,8 @@ namespace gr {
bool d_packed;
boost::crc_optimal<32, 0x04C11DB7, 0xFFFFFFFF, 0xFFFFFFFF, true, true> d_crc_impl;
int d_crc_length;
char *d_unpacked_crc;
std::vector<char> d_buffer;
unsigned int calculate_crc32(const unsigned char* in, size_t packet_length);

public:
crc32_bb_impl(bool check, const std::string& lengthtagname, bool packed);
Expand All @@ -48,8 +49,6 @@ namespace gr {
gr_vector_const_void_star &input_items,
gr_vector_void_star &output_items);

uint64_t d_npass;
uint64_t d_nfail;
};

} // namespace digital
Expand Down