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

Add support for AEPEX #675

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Add support for AEPEX #675

merged 1 commit into from
Oct 30, 2024

Conversation

git-addrian
Copy link

Per discussion here, adding gr-satellites support for the Atmospheric Effects of Precipitation through Energetic X-Rays (AEPEX) mission. The decoder created is for 70cm telemetry.

Copy link
Owner

@daniestevez daniestevez left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR. I have a few comments about coding style.

python/satyaml/AEPEX.yml Outdated Show resolved Hide resolved
python/telemetry/aepex_70cm.py Outdated Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Outdated Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Outdated Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Outdated Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Outdated Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Outdated Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Show resolved Hide resolved
python/telemetry/aepex_sw_stat.py Show resolved Hide resolved
@git-addrian
Copy link
Author

Hey @daniestevez , I have made the requested changes. Some notes:

  • In order to use LinearAdapter or AffineAdapter, I had to modify the coefficients. For example, if a previous version used PolynomialAdapter([0.0, c1], BitsInteger(8)), it had to be updated to be LinearAdapter(1/c1, BitsInteger(8)). I have gone through and updated each one to reflect it.
  • Packets without APID 0x01 are now handled by GreedyBytes. APID 0x01 is the packet that will beacon. At this time we do not intend to create a decoder for other APIDs.
  • Other feedback has been incorporated!

Best,
Adrian

@daniestevez
Copy link
Owner

Thanks @git-addrian! The code looks good now. However CI is failing. The error is ModuleNotFoundError: No module named 'satellites'. Sometimes I've seen Python errors in the satellites module cause this ModuleNotFoundError because of how it is imported within a try statement. The real error gets eaten up, which doesn't make the cause obvious.

Can you take at look at this? If this is working fine for you locally, I can take a look too. Maybe the CI pipeline is broken and would also fail for the main branch.

@daniestevez
Copy link
Owner

I found the problem by building locally and importing the python module from the build directory:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/daniel/gr-satellites/python/__init__.py", line 54, in <module>
    from .adsb_kml import adsb_kml
  File "/home/daniel/gr-satellites/python/adsb_kml.py", line 15, in <module>
    from .telemetry import gomx_3 as tlm
  File "/home/daniel/gr-satellites/python/telemetry/__init__.py", line 22, in <module>
    from .aepex_70cm import aepex_70cm
  File "/home/daniel/gr-satellites/python/telemetry/aepex_70cm.py", line 18, in <module>
    from .aepex_sw_stat import aepex_sw_stat
  File "/home/daniel/gr-satellites/python/telemetry/aepex_sw_stat.py", line 13, in <module>
    from .adapters import PolynomialAdapter, LinearAdapter
ModuleNotFoundError: No module named 'python.telemetry.adapters'

The problem is that this line

from .adapters import PolynomialAdapter, LinearAdapter

should say

from ..adapters import PolynomialAdapter, LinearAdapter

Can you fix this and test locally that the telemetry parser runs correctly afterwards (since there are no unit tests for it)?

@git-addrian
Copy link
Author

Hey @daniestevez ,
I was able to fix that line, build it, and confirm it was working with some different test telemetry. I also fixed the following:

  • AEPEX.yml was not pointing to its decoder, 'aepex_70cm'. This was silly, and was causing most of my issues with the local build.
  • I recieved corrections on which telemetry was supposed to be signed instead of unsigned. Some telemetry readings are now much more accurate.

Please let me know if there are any other changes to be made!

@daniestevez
Copy link
Owner

AEPEX.yml was not pointing to its decoder, 'aepex_70cm'.

Good point. I didn't notice this.

Copy link
Owner

@daniestevez daniestevez left a comment

Choose a reason for hiding this comment

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

Everything looks good now! Merging.

@daniestevez daniestevez merged commit 8b20318 into daniestevez:main Oct 30, 2024
3 checks passed
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.

2 participants