sot / chandra_aca

Chandra Aspect Camera Tools
https://sot.github.io/chandra_aca
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Fix maude_decom.get_raw_aca_packets #144

Closed javierggt closed 1 year ago

javierggt commented 1 year ago

Description

This PR fixes what I would argue is a bug in maude_decom.get_raw_aca_packets. The problem was that I was calling this function with unusual arguments (frames inconsistent with the time range).

This function takes start/stop times and (optionally) the output from maude.get_frames. Using those frames, it should return all the full ACA packets within that time range (remember that the ACA telemetry comes split in four VCDU frames).

In the master version, it groups the packages using the VCDU frame counter, but instead of getting the counter from the frames themselves, it uses the requested time range to query MAUDE. Then it does not check whether the given frames match the frame counters from the time range, and proceeds happily, producing garbage (combining frames in groups of four, but starting at the wrong one).

There is no reason why this function should fail with inconsistent arguments, because the frame counter is in the frames themselves. By fixing this, I am also removing this extra call to MAUDE that is not necessary.

Other changes

This PR includes two other changes I made while working on this. One was cause for another error. The other one was just a relatively simple change:

Interface impacts

None

Testing

A new unit test was added for this (this PR is two commits, the first one is the test, so you can verufy the test fails before the change).

Unit tests

Independent check of unit tests by Jean

Functional tests

I have been using this with aca_view (PR sot/aca_view/pull/169), and there I am making many functional tests.

javierggt commented 1 year ago

I added some comments to address Jean's review comments.

While I was at it, I also changed it so the image values are masked if the image type is DNLD, and updated the test to check for that.