CCSDSPy / ccsdspy

I/O interface and utilities for CCSDS binary spacecraft data in Python. Library used in flight missions at NASA, NOAA, and SWRI
https://ccsdspy.org
BSD 3-Clause "New" or "Revised" License
78 stars 19 forks source link

Integer parsing mismatch #76

Closed nischayn99 closed 1 year ago

nischayn99 commented 1 year ago

Hi @ddasilva ,

For this packet field,

ccsdspy.PacketField(f"FGx_CH{c}_{i}", bit_length=24, data_type='int')

When the expected value is positive, we're getting the correct result, but when it is negative, we're getting erroneous values. For example, when the expected value is -88, the value we're getting is 16777128. Could you please let us know what we could do?

Thanks, Nischay Nagendra

ddasilva commented 1 year ago

Hi @nischayn99 , can you provide a full example to reproduce, ideally as concise as possible? I can't really do anything with the amount of information you've given.

nischayn99 commented 1 year ago

Hi @ddasilva ,

Sorry for the inconvenience. I've attached the code and the test file. In the output xlsx file generated, when the expected value is positive, we're getting the correct result, but when it is negative, we're getting erroneous values. For example, when the expected value is -88, the value we're getting is 16777128 (In sheet APID_1227).

Thanks. Test.zip

ddasilva commented 1 year ago

Hi @nischayn99 ,

There was a small bug when integer fields are not a native byte length (eg, 4 bytes or 8 bytes). I made a fix and pushed it to this branch. I also wrote a regression test to check this doesn't happen in the future. https://github.com/CCSDSPy/ccsdspy/tree/fix-odd-length-neg-ints

Can you give this a try and let me know whether it resolves your issue?

nischayn99 commented 1 year ago

Hi @ddasilva ,

Still getting the same output. Also in the changes I don't see changes made to other files, only the test_regression file. Maybe the changes are not merged?

ddasilva commented 1 year ago

You're right, @nischayn99, I'll fix the branch and repush later this afternoon.

ddasilva commented 1 year ago

@nischayn99 Updated the branch: https://github.com/CCSDSPy/ccsdspy/commit/54d9fb58be78217e9a5bcf9b4b695b64929771a5

nischayn99 commented 1 year ago

Thank you, @ddasilva. The code is able to parse negative integers properly now.

ddasilva commented 1 year ago

Great, I will open a merge request for this shortly.

ddasilva commented 1 year ago

This was merged in #77. Closing this issue. Thanks for reporting!