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

42 redo sbn reintegrate #135

Merged
merged 20 commits into from
Jul 15, 2024

Conversation

williamzhang0306
Copy link
Contributor

This branch updates the sbn_adapter data source to the current state of OnAir and including unit tests.

@williamzhang0306
Copy link
Contributor Author

Should the files config/sbn_cfs_config and data.telemetry_configs/adapter_TLM_CONFIG.json stay in this branch? I'm pretty sure they were used for testing sbn_adapter with cfs at one point, but I don't know where that testing was done.

@williamzhang0306 williamzhang0306 marked this pull request as ready for review July 5, 2024 14:41
Copy link
Contributor

@the-other-james the-other-james left a comment

Choose a reason for hiding this comment

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

Overall this is looking really good! I had just a few questions / nitpicks.

Can you also document how you ran this with cFS? If this is short, you can add it to the top level readme, otherwise do it in a separate doc. For cFS, can you point me to the repo you set up and I'll fork it?

Great work, thanks!

onair/data_handling/sbn_adapter.py Show resolved Hide resolved
onair/data_handling/sbn_adapter.py Outdated Show resolved Hide resolved
onair/data_handling/sbn_adapter.py Outdated Show resolved Hide resolved
test/onair/data_handling/test_sbn_adapter.py Outdated Show resolved Hide resolved
@the-other-james
Copy link
Contributor

I copied these changes over to a setup I have for a different project, and can confirm that it functions as expected and the OnAIR results were identical with the older SBN rework I did.

@williamzhang0306
Copy link
Contributor Author

Overall this is looking really good! I had just a few questions / nitpicks.

Can you also document how you ran this with cFS? If this is short, you can add it to the top level readme, otherwise do it in a separate doc. For cFS, can you point me to the repo you set up and I'll fork it?

Great work, thanks!

Thank you! This is the repo I used for cfs: https://github.com/williamzhang0306/cFS-OnAIR.

@jlucas9
Copy link
Contributor

jlucas9 commented Jul 11, 2024

Note that we are also including OnAir in NOS3 which includes a distribution of cFS draco-rc4 at the time of writing:

We made forks for now trying to get things working:

I can add you both to the repos if you'd like to assist directly there and collaborate more closely!

Copy link
Contributor

@the-other-james the-other-james left a comment

Choose a reason for hiding this comment

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

This is great work! I tried out the instructions and they worked flawlessly on my machine. Thanks!

Just a few small tweaks, and I want @asgibson to have a chance to review the unit tests before final approval.

README.md Outdated Show resolved Hide resolved
doc/cfs-onair-guide.md Show resolved Hide resolved
doc/cfs-onair-guide.md Outdated Show resolved Hide resolved
doc/cfs-onair-guide.md Outdated Show resolved Hide resolved
{"0x0887": ["SAMPLE", "sample_data_power_t"],
"0x0889": ["SAMPLE", "sample_data_thermal_t"],
"0x088A": ["SAMPLE", "sample_data_gps_t"]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot newline

Copy link
Contributor

@asgibson asgibson left a comment

Choose a reason for hiding this comment

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

Unit tests look good enough.

@the-other-james the-other-james merged commit 9fb9276 into nasa:main Jul 15, 2024
5 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.

None yet

4 participants