-
Notifications
You must be signed in to change notification settings - Fork 93
1150 simple antenna #1229
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
base: develop
Are you sure you want to change the base?
1150 simple antenna #1229
Conversation
16ade02 to
2d173c9
Compare
schaubh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, very nice work overall. Some small things to look at.
Also, you don't have release notes added. Please add a brief statement to docs/source/Support/bskReleaseNotes.rst saying this branch adds :ref:simpleAntenna and :ref:antennaPower modules.
| /*! antenna state message definition */ | ||
| typedef struct { | ||
| char antennaName[20]; //!< -- Antenna Name | ||
| char ModelTag[20]; //!< -- Model tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have ModelTag in the message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linkBudget module is going to have two modules: "simpleAntenna" attached.
To distinguish one module: "simpleAntenna" from the other one we either need the ModelTag or the antennaName.
I was not aware of the ModelTag until you (@schaubh ) made me aware of this.
What do you think is better, keeping ModelTag or antennaName for this purpose (or both?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think antennaName name here is clearer. Thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I "re-introduced" antennaName and removed modelTag. antennaName is a mandatory value (BSK_ERROR) if not set.
The thought behind this is, that someone might want to have >1 antenna on a spacecraft and therefore must be able to keep them appart.
I have done the related changes in the following files:
- simpleAntenna.cpp
- simpleAntenna.h
- AntennaLogMsgPayload.h
Leaving this conversation unresolved. If you are happy with the change feel free to 'resolve conversation'
| "supportData/LocalGravData/VESTA20H.txt": "md5:f9ec4308ea7049656f64e64d6019d570", | ||
| "supportData/LocalGravData/eros007790.tab": "md5:5bdf62cee6c7069ea547cca6ed7c724f", | ||
| "supportData/MagneticField/WMM.COF": "md5:8caac788545d3a32d55faba8aa87c93b", | ||
| "supportData/AlbedoData/Earth_ALB_2018_CERES_All_10x10.csv": "md5:a448e61eadaebc76b4a03d612b1d6bbf", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ReeceHumphreys , does this look correct to you? Thanks for the quick sanity check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there may be an issue with that @ReeceHumphreys. When I run the bskLargeData command. It says:
`...
Downloading supportData/LocalGravData/VESTA20H.txt
Downloading supportData/LocalGravData/eros007790.tab
Downloading supportData/MagneticField/WMM.COF
Downloading supportData/SkyBrightnessData/haslam408_dsds_Remazeilles2014.fits
Fetching data files: 100%|█| 50/50 [00:00<00:00, 290.09file/s]
Completed pre-fetch.
Fetched 50 / 50 files.
All supportData files fetched successfully!
(.venv) dahu1128@UCB-GWYQFGP29F basilisk % cd supportData
(.venv) dahu1128@UCB-GWYQFGP29F supportData % ls
AlbedoData AtmosphereData DentonGEO EphemerisData LocalGravData MagneticField
`
The folder "SkyBrightnessData" does not get created and the Haslam Map is actually not being downloaded...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hashmap should not be getting updated at all since this file is being fetched from an external source.
The changes to this seem to be related to the windows file endings stuff we were dealing with last week. I might have to send out instructions to anyone on windows to run on their computers so this isn't an issue.
I can investigate more later in the week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked and the file downloaded for me checking out this PR, you may not have been looking in the right place. Pooch caches the files itself in its own caches:
~/Developer/AVS/basilisk 1150-simpleAntenna* ⇣⇡
❯ cd ~/Library/Caches/bsk_support_data
~/Library/Caches/bsk_support_data
❯ ls
supportData
~/Library/Caches/bsk_support_data
❯ cd supportData
~/Library/Caches/bsk_support_data/supportData
❯ l
total 0
drwxr-xr-x 9 reece staff 288B Jan 20 15:02 .
drwxr-xr-x 3 reece staff 96B Jan 20 15:02 ..
drwxr-xr-x 20 reece staff 640B Nov 30 16:30 AlbedoData
drwxr-xr-x 11 reece staff 352B Nov 30 16:30 AtmosphereData
drwxr-xr-x 10 reece staff 320B Nov 30 16:30 DentonGEO
drwxr-xr-x 10 reece staff 320B Jan 2 02:03 EphemerisData
drwxr-xr-x 7 reece staff 224B Nov 30 16:30 LocalGravData
drwxr-xr-x 3 reece staff 96B Nov 30 16:30 MagneticField
drwxr-xr-x 3 reece staff 96B Jan 20 15:02 SkyBrightnessData
~/Library/Caches/bsk_support_data/supportData
❯ cd SkyBrightnessData
~/Library/Caches/bsk_support_data/supportData/SkyBrightnessData
❯ ls
haslam408_dsds_Remazeilles2014.fits| "supportData/EphemerisData/de-403-masses.tpc": "https://naif.jpl.nasa.gov/pub/naif/generic_kernels/pck/de-403-masses.tpc", | ||
| "supportData/EphemerisData/hst_edited.bsp": "https://naif.jpl.nasa.gov/pub/naif/HST/kernels/spk/hst_edited.bsp", | ||
| "supportData/EphemerisData/nh_pred_od077.bsp": "https://naif.jpl.nasa.gov/pub/naif/pds/data/nh-j_p_ss-spice-6-v1.0/nhsp_1000/data/spk/nh_pred_od077.bsp", | ||
| "supportData/SkyBrightnessData/haslam408_dsds_Remazeilles2014.fits": "https://lambda.gsfc.nasa.gov/data/foregrounds/haslam_2014/haslam408_dsds_Remazeilles2014.fits", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran all the tests, but this data never got downloaded to my computer. Where is this data used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there are a few Unit tests missing.
I will look into this and add them myself or coordinate with Daniel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have too much time today to investigate this in detail but I see the skyTemperature408MHz is never used in this PR. If a test does not use this data then the file will never be downloaded since everything is on demand now.
There would need to be this in a test for it to automatically be pulled.
some_variable = get_path(DataFile.SkyBrightnessData.skyTemperature408MHz)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The haslam map is not tested (cheers to @dhutererprats)
please 'resolve conversation' if you are happy with this.
| @@ -0,0 +1,98 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the 1st line of the commit message is too long. GitHub uses the first line as the title. Please edit the commit message to have a shorter first line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have shortened the first line of all the commit messages.
Please let me know if there are still issues regarding this.
please 'resolve conversation' if this is good.
2d173c9 to
db5e125
Compare
e43e002 to
13bcec7
Compare
| @@ -1,4 +1,4 @@ | |||
| # Auto-generated registry of support data files by make_registry.py | |||
| # Auto-generated registry of support data files by makeRegistry.py | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it weird that the hashes in this file have changed order. I may have made some minor tweaks makeRegistry and not committed the resulting registry. I'll look into it but this PR shouldn't need to to regenerate the registry since we are not versioning the data file ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working now.
From my side this can be closed.
| "supportData/EphemerisData/de-403-masses.tpc": "https://naif.jpl.nasa.gov/pub/naif/generic_kernels/pck/de-403-masses.tpc", | ||
| "supportData/EphemerisData/hst_edited.bsp": "https://naif.jpl.nasa.gov/pub/naif/HST/kernels/spk/hst_edited.bsp", | ||
| "supportData/EphemerisData/nh_pred_od077.bsp": "https://naif.jpl.nasa.gov/pub/naif/pds/data/nh-j_p_ss-spice-6-v1.0/nhsp_1000/data/spk/nh_pred_od077.bsp", | ||
| "supportData/SkyBrightnessData/haslam408_dsds_Remazeilles2014.fits": "https://lambda.gsfc.nasa.gov/data/foregrounds/haslam_2014/haslam408_dsds_Remazeilles2014.fits", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have too much time today to investigate this in detail but I see the skyTemperature408MHz is never used in this PR. If a test does not use this data then the file will never be downloaded since everything is on demand now.
There would need to be this in a test for it to automatically be pulled.
some_variable = get_path(DataFile.SkyBrightnessData.skyTemperature408MHz)
10b7a01 to
4a85854
Compare
|
Further changes done since the review 1:
|
954659e to
3453d7a
Compare
This commit contains the main (*.cpp), header (*.h) and swig (*.i) files for these modules. Further changes which have been done in this commit: - Introduction of the message: "AntennaStateMsgPayload.h" containing the antenna state (Off, Rx, Tx, RxTx) - Introduction of the message: "antennaLogMsgPayload.h" containing the antenna state (will be an input to link-budget calculations) - Introduction of an antenna type in: "_GeneralModuleFiles/AntennaDefinitions.h" - Update to the build system: "conanfile.py", "CMakeLists.txt" and "utilities/CMakeKLists.txt"
The following libraries were added: - brightnessTemperatureSolarSystem - haslamBackgroundRadiation. These libraries are used to estimate the sky brighness temperature seen by the antenna. Together with this library, the registrySnippet.py was updated (needed for the data downloaded in SupportData)
ab50738 to
d3dcf2e
Compare
- Renaming v_AN to v_AN_N - Update to cpp and h code comments to improve documentation (remove rendering errors)
d55d3f0 to
2cf6cdc
Compare
- Fixes related to hash generation - Haslam map file path
- Removal of not needed/outdated comments - Corrections to existing comments
2cf6cdc to
4233b54
Compare
|
@schaubh This PR is ready for review again.
|
Description
The simpleAntenna module allows attaching a simplified antenna model to a spacecraft or to a ground location. This forms the basis for a link-budget which shall be implemented on a later stage.
Verification
Unit tests for the modules simpleAntenna and antennaPower
Documentation
Documentation for both modules has been added to the basilisk framework
Future work
Implementation of the module "link budget"