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

Add straightforward endianness implementation; accommodate all allowable string encodings #26

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cgobat
Copy link

@cgobat cgobat commented Mar 18, 2024

This PR ended up turning into a kind of rollup of several of the changes I've made in my fork to make the module usable. I am opening this as a draft for discussion purposes and as a sort of already-implemented feature(s) request. The major changes here are:

  • Addition of a BooleanParameterType class
  • Implementation of byte ordering/endianness checking
    • While in an ideal world, spacecraft subsystems, etc. would all stick to big endian data types, in practice that is not the case. Hardcoding big endian types for all parameters is not a safe assumption, and the ability to handle both MSB- and LSB-first fields is a pretty fundamental/core functionality
    • I noticed several notes in the code along the lines of TODO: implement ByteOrderList to inform endianness. I'm not sure what "ByteOrderList" is a reference to (a name idea for a new/future class?), but the way I've done it here is pretty clean (IMO) and seems to be fully transparent and backwards compatible in my limited testing. Happy to discuss if you had something else in mind for this though.
  • Addition of the complete set of XTCE-allowable character encoding values for StringDataEncodings, plus integration with the aforementioned endianness parsing for the cases where byte order is described by the string encoding

Checklist

  • Changes are fully implemented without dangling issues or TODO items
  • Deprecated/superseded code is removed or marked with deprecation warning
  • Current dependencies have been properly specified and old dependencies removed
    • Note: I still think it is worth it to add an lxml dependency, but I will leave that for another PR
  • New code/functionality has accompanying tests and any old tests have been updated to match any new assumptions
  • The changelog.md has been updated

@medley56
Copy link
Owner

@cgobat This is great work and I want to get these changes in. I think I will do the same thing as your last PR and take these changes and incorporate them into a different changeset that includes tests and omits a few pieces (e.g. I'm still torn on incorporating the lxml dependency and a lot of tests will need to be reworked for that I think). If you're concerned about claiming "credit" for the changes, I can try to teach myself how to continue work on your branch. Is it as simple as me making commits to your fork?

The BooleanParameterType was part of PR #24, along with the time parameter types so I think we'll pull those changes out of this PR.

On the subject of the ByteOrderList element, the XTCE green book element description is a little unclear on it. I think in XTCE 1.1 there was a ByteOrderList element that literally spelled out the byte significant for every byte in a data encoding. However, it looks like in 1.2 this may have been superseded by the mostSignificantByteFirst and leastSignificantByteFirst attributes that you are using. The explicit element-form of ByteOrderList appears to be still allowed in XTCE 1.2 but seems like an extremely niche use-case now that we have the attribute forms instead. In short, I think your implementation is sufficient.

See https://issues.omg.org/issues/XTCE12-400 for a discussion of the byte ordering changes between XTCE 1.1 and 1.2.

@cgobat
Copy link
Author

cgobat commented Mar 19, 2024

If you're concerned about claiming "credit" for the changes, I can try to teach myself how to continue work on your branch. Is it as simple as me making commits to your fork?

It's not a huge deal to me. It might be nice, but only if it's not too inconvenient for you and your workflow. That said, GitHub does make it pretty easy to continue working on this branch—just git fetch origin pull/26/head:pr26 (assuming your remote is called origin; you can change pr26 to be whatever you want to call your own branch), then check out that new branch (e.g. git switch pr26). Since I enabled maintainer edits on this PR, you should be able to push.

The BooleanParameterType was part of PR #24

Ah, I had missed that, thanks. I removed the duplicate definition here.

See https://issues.omg.org/issues/XTCE12-400 for a discussion of the byte ordering changes between XTCE 1.1 and 1.2.

Thanks for the link. That provides some good insight.

Cheers!

@medley56
Copy link
Owner

Awesome! Thanks for the tip on checking out PR branches from forks. I've always struggled with that.

The last piece here is unit test coverage for these new features. Do you want to tackle that? If not, I can start working on it, adding commits to this branch.

@cgobat
Copy link
Author

cgobat commented Mar 22, 2024

I am pretty tight for time these days. I think it will definitely be ready sooner if you go ahead get started on the tests.

@medley56
Copy link
Owner

Ok. I'll take over this PR branch and start working on tests.

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

2 participants