us-irs / spacepackets-rs

CCSDS and ECSS packet standards implementations
https://egit.irs.uni-stuttgart.de/rust/spacepackets
Apache License 2.0
22 stars 3 forks source link

Question about Panics in spacepackets-rs #2

Closed EthanJamesLew closed 7 months ago

EthanJamesLew commented 1 year ago

Hello!

I've been quite interested in a rust implementation of spacepackets and have enjoyed using your project.

I have been grokking the codebase of the Rust spacepacket implementation and noticed the use of panics in the builder methods, specifically in the const_new and new functions of various structs. While I understand that panics are a part of Rust's error handling mechanism and can be useful for catching exceptional cases, I have concerns regarding their usage in these particular scenarios.

First, in the UnixTimestamp struct, the const_new function panics if the subsec_millis value exceeds 999. Similarly, in the PacketSequenceCtrl struct, both the const_new and new functions panic if the seq_count value exceeds MAX_SEQ_COUNT. Although panics can be appropriate for handling unexpected or invalid inputs, I'm unsure whether they are the best approach in these cases. I do understand for the latter example that these panic conditions are checked before calling, so they won't panic in those cases.

Thank you,

Ethan

robamu commented 1 year ago

Hmm, I don't think const_new is something that common. This is just a variant which allows creation of structs in const contexts, which might be useful for some cases. The caveat here of course is that I can not perform regular error handling, so I don't really have any other choice than to panic on invalid/unsanitized input. The new APIs should not panic however, do you have concrete examples?

About the subsec_millis being greater than 999: It is assumed that the subsec_millis only contain the millisecond part on top of the full UNIX seconds, so a value greater than 999 would be an additional second.

robamu commented 7 months ago

Big changes for everything you mentioned are coming in version 0.11.

I renamed the const_new methods to new and renamed the former new methods to new_checked. This is more in line with how it is done in the Rust ecosystem. The new UnixTime timestamp type now consists of a second part (i32) and a nanoseconds part (i64). The same rules still apply though: A nano second value larger or equal to 1_000_000_000 is considered invalid.

EthanJamesLew commented 7 months ago

@robamu great to know, thanks!

Also, I will close this issue as my questions have been answered (a long time ago :) )