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

Implement support for AOS frames #65

Closed
wants to merge 0 commits into from
Closed

Implement support for AOS frames #65

wants to merge 0 commits into from

Conversation

kmarwah
Copy link
Contributor

@kmarwah kmarwah commented Jun 21, 2019

Resolves #60

@kmarwah kmarwah requested review from a team as code owners June 21, 2019 17:50
@aywaldron aywaldron self-assigned this Jun 21, 2019
Copy link
Contributor

@aywaldron aywaldron left a comment

Choose a reason for hiding this comment

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

@kmarwah Thanks for the PR! Overall it looks good, there just a few updates I'd like to see that I've left comments for. If you don't have time to do them let me know and I can do them after merging this PR.

responder_id: dsn_hmap
peer_password: lunahmap
version: 4
telemetryframetype: AOS
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the project specific values from this PR, and change the parameter name and default type to downlink_frame_type: TMTransFrame

@@ -334,7 +334,12 @@ def _transfer_data_invoc_handler(self, pdu):
ait.core.log.info(err)
return

tmf = frames.TMTransFrame(tm_data)
if self._telemetryframetype == "TM":
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be cleaner if we require the type passed in to be a valid class name rather than an abbreviation and then check dynamically for the class in frames.py. That way we don't have to edit this code in the future when additional frame types are added. Then this chunk can be changed to

tmf = getattr(frames, self._downlink_frame_type)

@@ -409,7 +409,12 @@ def _transfer_data_invoc_handler(self, pdu):
ait.core.log.info(err)
return

tmf = frames.TMTransFrame(tm_data)
if self._telemetryframetype == "TM":
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

@@ -97,6 +97,8 @@ class SLE(object):

def __init__(self, *args, **kwargs):
''''''
self. _telemetryframetype = ait.config.get('dsn.sle.telemetryframetype',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update the attribute name, parameter name, and default to _downlink_frame_type, downlink_frame_type, and TMTransFrame respectively.

@@ -164,6 +166,7 @@ def add_handler(self, event, handler):
def send(self, data):
''' Send supplied data to DSN '''
try:

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this added by mistake?

return

#offset to first byte of ccsds header (2047 means no data)
if self['first_hdr_ptr'] == b'11111111111':
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where self['first_hdr_ptr'] is being assigned before this.

@aywaldron
Copy link
Contributor

Requested changes in PR https://github.com/NASA-AMMOS/AIT-DSN/pull/69

@aywaldron aywaldron closed this Jun 26, 2019
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.

Implement AOS Transfer Frames
2 participants