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

Fix up edl_cmd_shell for os commands #44

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Conversation

ThirteenFish
Copy link
Contributor

When invoked as "sdo_write c3 os_command command ..." this patch fixes two thing:

  • ... can now have spaces in it
  • command is defined as DOMAIN which we didn't handle. There's some amount of stuff built around the assumption that we don't use DOMIAIN at all but this is what we shipped so we've got to support it now. Amusingly it looks like pycanopen's encode_raw() has a bug where it doesn't convert strings to bytes correctly so it's done manually here.

When invoked as "sdo_write c3 os_command command ..." this patch fixes
two thing:
- ... can now have spaces in it
- command is defined as DOMAIN which we didn't handle. There's some
  amount of stuff built around the assumption that we don't use DOMIAIN
  at all but this is what we shipped so we've got to support it now.
  Amusingly it looks like pycanopen's `encode_raw()` has a bug where it
  doesn't convert strings to bytes correctly so it's done manually here.
cfdp-py has updated with handy bug fixes! Unfortunately those conflict
with our fixes so until we can properly ship a new version to space
let's fix the version to what we've been developing with.
Similar to cmd_shell and ping, this makes it easy to coordinate with
those other scripts.
Copy link
Member

@ryanpdx ryanpdx left a comment

Choose a reason for hiding this comment

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

If sending bash commands via edl becomes common, we may want to make a edl os command script

@@ -284,8 +284,10 @@ def do_sdo_write(self, arg: str):
value = float(args[3])
elif obj.data_type == canopen.objectdictionary.VISIBLE_STRING:
value = args[3]
elif obj.data_type == canopen.objectdictionary.DOMAIN:
value = args[3].encode("ascii")
Copy link
Member

Choose a reason for hiding this comment

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

This assume the value is string, domain type are void, so this wont work in all cases. It would be more robust to expect the user to past a hex string for domain types.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the quick encode a string will work for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point, I would prefer to do it properly over quick hacks. Looking through CiA-301 and oresat-configs it feels like while officially undefined the intended use for DOMAIN is for file transfers; if the data is short enough to be written as a hex string easily it should generally be OCTET_STRING. What if instead for DOMAIN we expect a filename and do_sdo_write() opens and sends the contents?

On the other hand it looks like the type is wrong for OS command in oresat-configs. The command should be OCTET_STRING instead of DOMAIN (CiA-301 7.4.8.6). Additionally 7.5.2.26 says command should be "ISO8859 [ASCII] characters or binary". So given that, what if in do_sdo_write() for OCTET_STRING we try to decode the argument from a hex string to bytes and if that fails then just .encode("ascii") the given string? That way we cans still send shell commands directly through sdo_write.

In code:

...
elif obj.data_type == canopen.objectdictionary.DOMAIN:
    with open(arg[3], 'rb') as f:
        value = f.read()
elif obj.data_type == canopen.objectdictionary.OCTET_STRING:
    try:
        value = bytes.fromhex(arg[3])
    except ValueError:
        value = arg[3].encode("ascii")

Copy link
Member

Choose a reason for hiding this comment

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

Yea, that sounds like a good plan

In 5f961ed I added DOMAIN handling so that we could send os commands,
but DOMAIN really is supposed to be for file-like things. This enhances
sdo_write to interpret the final argument instead as a filename, which
it then opens and sends.

Additionally os commands really should have been of type OCTET_STRING
but that needs to be fixed in oresat-configs. This adds OCTET_STRING
handling in anticipation of that.
@ThirteenFish ThirteenFish merged commit 024522f into master Oct 8, 2024
1 check passed
@ThirteenFish ThirteenFish deleted the edl-os-cmd-fix branch October 8, 2024 19:21
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