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

optional secondary header #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

patrickoppel
Copy link

One option to do optional secondary header,
alternatively maybe a macro?

@MoralCode
Copy link
Member

Took a brief look while mobile and looks good! Will look at it in some more detail when I get a chance to run it.

Maybe this is my relative inexperience with rust, but do you always need to specify the value of the S generic even if you dont need to parse the secondary header?

Also, this is one part I had trouble with when i tried to add something like this way back when: the CCSDS spec allows either a payload, or a secondary header, or a secondary header and a payload. Is it going to be easy to allow the payload to also be optional, but in a way thay both cannot be none at the same time?

Also ultimately it would be nice to have unit tests to demonstrate usage and validate that it works as intended. I can add those later though.

Thanks so much for your contribution!

@patrickoppel
Copy link
Author

Yeah, I'm just working on the tests to see how well it works. At the moment this only works if you specify a Secondary Header.

Re optional payload: I think the payload vector can just be left empty if no payload is expected

@MoralCode
Copy link
Member

Oh also, i noticed theres a phantomdata field and was wondering what the purpose of that was. Should have included that in my last comment too

@MoralCode
Copy link
Member

At the moment this only works if you specify a Secondary Header.

No worries, in that case my plan was to just have a Premade empty header struct that people can pass in if they have no secondary header. Or maybe an overloaded method that does that for you (if rust supports that).

@patrickoppel
Copy link
Author

Oh also, i noticed theres a phantomdata field and was wondering what the purpose of that was. Should have included that in my last comment too

It's needed for the lifetime parameter. Otherwise the compiler complains that it is not used.

Or maybe an overloaded method that does that for you (if rust supports that).

From my experience it doesn't, haven't really tried it with functions, but for structs etc it doesn't allow that

@patrickoppel
Copy link
Author

Overloading is possible with macros though. Not sure if we can achieve 100% what you're imagining

@MoralCode
Copy link
Member

MoralCode commented Dec 17, 2021

Nah, overloading was just an idea. No big deal if its not possible with functions

@MoralCode
Copy link
Member

Well i totally forgot about this. You mentioned you were working on tests. Do you have any unit test cases to help demo the functionality?

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