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

Support arbitrary byte sequence containers #3

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

Conversation

robinkrahl
Copy link
Contributor

Previously, the Command and Response types always used heapless::Vec to store the payload. This was awkward to use in non-embedded environments where std::vec::Vec might be preferable, or in situations where a slice is sufficient.

This patch relaxes this requirement and accepts any type implementing AsRef<[u8]> (and TryFrom<&[u8]> for construction).

Previously, the Command and Response types always used heapless::Vec to
store the payload.  This was awkward to use in non-embedded environments
where std::vec::Vec might be preferable, or in situations where a slice
is sufficient.

This patch relaxes this requirement and accepts any type implementing
AsRef<[u8]> (and TryFrom<&[u8]> for construction).
@robinkrahl
Copy link
Contributor Author

For reference, here is an updated version of apdu-dispatch that uses this branch: robinkrahl/apdu-dispatch@cffd58a

pub type Result<T=()> = core::result::Result<T, Status>;

pub mod aid;
pub mod command;
pub mod response;
pub mod somebytes;
Copy link
Member

Choose a reason for hiding this comment

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

Rename to data?

Copy link
Member

Choose a reason for hiding this comment

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

Also generally wondering if we should have all these public submodules; assuming they're just implementation, we could pub use the core data structures + traits at crate level and let the submodules be non-public.

@@ -10,12 +10,12 @@ pub enum Interface {
Contactless,
}

pub type Data<const S: usize> = heapless::Vec<u8, S>;
Copy link
Member

Choose a reason for hiding this comment

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

pub use data::Data (where data::Data is the current somebytes::Bytes)? There's already a bunch of Bytes types in the eco-system, if we're rolling our own it would seem to make sense to name it semantically.

///
/// This wrapper implements common traits based on the content of the byte sequence.
#[derive(Clone)]
pub struct Bytes<T: AsRef<[u8]>>(T);
Copy link
Member

Choose a reason for hiding this comment

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

I suggest pub struct Data<B: AsRef<[u8]>>(B) here. You're also using the B elsewhere (pointing to it meaning Bytes).

#[inline]
fn into(self) -> Data<S> {
fn into(self) -> heapless::Vec<u8, S> {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an inherent into_vec method (in case it doesn't already exist).

fn extend_from_slice(&mut self, slice: &[T]) -> Result<(), Self::Error>;
}

impl<T: Clone, const N: usize> TryExtendFromSlice<T> for heapless::Vec<T, N> {
Copy link
Member

@nickray nickray Jun 1, 2022

Choose a reason for hiding this comment

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

I think it would make sense to have a alloc feature on the crate, and conditionally implement TryExtendFromSlice for alloc::vec::Vec?

}
}

impl<T: AsRef<[u8]>> fmt::Debug for Bytes<T> {
Copy link
Member

@nickray nickray Jun 1, 2022

Choose a reason for hiding this comment

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

Might be nice to have the Debug display hex? We already depend on delog, which re-implements hex-fmt.

Not necessarily for this PR, but it would be nice to have a serde feature, with at least a corresponding serde::ser implementation (as this can't be done outside this crate). Even nicer if it outputs hex if serializer.is_human_readable(), else actual bytes.

@@ -12,7 +12,8 @@ documentation = "https://docs.rs/iso7816"

[dependencies]
delog = "0.1.2"
heapless = "0.7"
# use unreleased heapless for https://github.com/japaric/heapless/pull/255
heapless = { git = "https://github.com/japaric/heapless" }
Copy link
Member

Choose a reason for hiding this comment

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

Seems this is merged, maybe also published?

sosthene-nitrokey added a commit to sosthene-nitrokey/iso7816 that referenced this pull request Jun 30, 2023
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