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

Proposed SatnogsTransferService to perform deduplication on Satnogs Telemetry API Result Field #62

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mattahmad
Copy link

This feature includes a new service called SatnogsTransferService which checks if data received from the Satnogs Telementry API result field is unique before sending the data as telemetry.

The following is a high level list of files which were changed:

pom.xml: jackson dependencies added to support parsing of results field from API response.
Dependency jars are available at https://mvnrepository.com/artifact/com.fasterxml.jackson.core/jackson-core/2.15.2
https://mvnrepository.com/artifact/com.fasterxml.jackson.core/jackson-annotations/2.15.2
https://mvnrepository.com/artifact/com.fasterxml.jackson.core/jackson-databind/2.15.2

SatnogsTmLink.java: Performs GET request from Satnogs Telemetry API, parses results field and calls getDeduplication in Satnogs TransferService.

SatnogsTransferService.java: Service file similar to UniclogsEnvironment which also contains a getDeduplication method to check for unique data.

SatnogsCommandPostprocessor.java was added similar to EdlCommandPostprocessor in case the Satnogs Telemetry need a way of pushing the data. This file can always be removed if it is not needed or updated as needed.

SatnogsPacket.java was added similar to EDLPacket.

SatnogsPacketPreprocessor.java was added similar to EdlPacketPreprocessor.

processor.yml, yamcs.oresat0.yaml, oresat.xml: Satnogs service configs were updated in files.

Copy link
Member

@dmitri-mcguckin dmitri-mcguckin left a comment

Choose a reason for hiding this comment

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

This PR is in in the right direction but still needs a lot of work.

If you have free time either Friday or Saturday, it may be beneficial to do additional revisions in a pair-programming session, because some intentions may have been lost in translation.

@@ -2,7 +2,7 @@ realtime:
services:
- class: org.yamcs.StreamTmPacketProvider
args:
streams: ["edl_tm_realtime", "beacon_tm_realtime", "tm_dump"]
streams: ["edl_tm_realtime", "beacon_tm_realtime", "tm_dump", "satnogs_tm_realtime"]
Copy link
Member

Choose a reason for hiding this comment

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

Satnogs Telemetry should be on the same caedance of beacon telemetry and thus should use the beacon_tm_realtime processor.

envService: SatnogsTransferService

- name: satnogs-tm-realtime
class: org.yamcs.tctm.UdpTmDataLink
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused here.

I see that you made org.oresat.uniclogs.links.SatnogsTmLink but you are using the generic UdpSocket link here. Is there a reason for that?

hmacEnvVar: HMAC_KEY
- class: org.oresat.uniclogs.services.SatnogsTransferService
args:
hmacEnvVar: HMAC_KEY
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you're including all of the same fixings of the UniclogsEnvironment service to Satnogs?

Satnogs will not be receiving or dealing with our HMAC keys whatsoever, so I think this needs to be removed.

@@ -0,0 +1,47 @@
package org.oresat.uniclogs.tctm;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this exists.

SatNOGS is a receive-only network, meaning we will never send commands to it and it has no capability to do commanding.

This plus anything else to do with commanding in this PR can be axed.


URL urlArtifacts = new URL("https://db.satnogs.org/api/telemetry/?format=json&satellite=98867");
HttpURLConnection conArtifacts = (HttpURLConnection) urlArtifacts.openConnection();
conArtifacts.setRequestProperty("Authorization", "Bearer " + System.getenv("SATNOGSDB_AUTH_TOKEN"));
Copy link
Member

Choose a reason for hiding this comment

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

Any environment variables should be managed via a higher level and not-baked into the code like this.

We've been going back-and-forth on the best way to do this, but ultimately, we're thinking it may have to be set as an ENV variable name set in yamcs.oresat*.yml and then the code uses that variable for the getenv call.

For now I'd say do this.

@@ -0,0 +1,31 @@
package org.oresat.uniclogs.tctm;
Copy link
Member

Choose a reason for hiding this comment

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

This can be culled as SatNOGS does not produce a unique TLM frame that we are processing in the Mission Database.

hmacEnvVar: HMAC_KEY
hmacEnvVar: HMAC_KEY
- class: org.oresat.uniclogs.services.SatnogsTransferService
args:
Copy link
Member

Choose a reason for hiding this comment

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

Add params like Satellite ID here, but remove HMAC.

stream: satnogs_tc_realtime
host: localhost
port: 10017
commandPostprocessorClassName: org.oresat.uniclogs.tctm.SatnogsCommandPostprocessor
Copy link
Member

Choose a reason for hiding this comment

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

Addressed in other comments, but this becomes an extraneous post-processor once all the CMD components are axed.

stream: satnogs_tm_realtime
host: localhost
port: 10018
packetPreprocessorClassName: org.oresat.uniclogs.tctm.SatnogsPacketPreprocessor
Copy link
Member

Choose a reason for hiding this comment

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

This can also be axed, as we are not processing a unique TLM frame from satnogs that our MDB recognizes.

@@ -286,6 +286,16 @@

</EntryList>
</SequenceContainer>
<SequenceContainer name="Satnogs_Packet">
Copy link
Member

Choose a reason for hiding this comment

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

In general, don't modify the XTCE because this file is autogenerated.

We should probably be adding it to our .gitignore, so it doesn't cause confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Implementing
Development

Successfully merging this pull request may close these issues.

2 participants