Skip to content

More robust handling of responses from NINA-FW #72

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.

Sign up for GitHub

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

Merged
jhodapp merged 12 commits into from
Feb 17, 2023
Merged

More robust handling of responses from NINA-FW #72

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7a3af36
Use core::hint to implement friendlier spin loop for gpio level-checking
jhodapp Feb 13, 2023
bbbe78a
Allows for different size param buffer lengths that takes advantage o…
jhodapp Feb 14, 2023
f5320f1
remove NinaNoParams
calebbourg Feb 15, 2023
46f0389
only read bytes equal to what NINA Firmware tells us
calebbourg Feb 15, 2023
0453761
Make sure that bytes passed into creating a new NinaParam* do not exc…
jhodapp Feb 15, 2023
bbfcef4
Make sure that bytes passed into creating a new NinaParam* do not exc…
jhodapp Feb 15, 2023
bd570f4
add tests to protocol.rs
calebbourg Feb 16, 2023
42a23c9
refactor receive function
calebbourg Feb 16, 2023
33ebdfa
cargo fmt
calebbourg Feb 16, 2023
ad4d6ee
improve internal ergonomics
calebbourg Feb 17, 2023
ed641f7
Fix clippy warning about needless_range_loop being used
jhodapp Feb 17, 2023
fa7cab9
Merge branch 'main' into more_robust_wait_response_cmd
jhodapp Feb 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 74 additions & 16 deletions esp32-wroom-rp/src/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,17 @@ pub(crate) enum NinaCommand {
SendDataTcp = 0x44,
}

pub(crate) trait NinaConcreteParam {
pub(crate) trait NinaConcreteParam
where
Self: core::marker::Sized,
Copy link
Collaborator

@calebbourg calebbourg Feb 16, 2023

Choose a reason for hiding this comment

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

Why was this necessary?
For my own education.

Copy link
Member Author

@jhodapp jhodapp Feb 17, 2023

Choose a reason for hiding this comment

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

Otherwise Self won't have a known size and you get this error:

error[E0277]: the size for values of type `Self` cannot be known at compilation time
   --> esp32-wroom-rp/src/protocol.rs:58:27
    |
58  |     fn new(data: &str) -> Result<Self, Error>;
    |                           ^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
note: required by a bound in `Result`
   --> /Users/jhodapp/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:504:17
    |
504 | pub enum Result<T, E> {
    |                 ^ required by this bound in `Result`
help: consider further restricting `Self`
    |
58  |     fn new(data: &str) -> Result<Self, Error> where Self: Sized;
    |                                               +++++++++++++++++

{
type DataBuffer;
// Length of parameter in bytes
type LengthAsBytes: IntoIterator<Item = u8>;

fn new(data: &str) -> Self;

fn from_bytes(bytes: &[u8]) -> Self;
fn from_bytes(bytes: &[u8]) -> Result<Self, Error>;

fn data(&self) -> &[u8];

Expand Down Expand Up @@ -176,14 +179,17 @@ impl NinaConcreteParam for NinaByteParam {
}
}

fn from_bytes(bytes: &[u8]) -> Self {
let mut data_as_bytes: Self::DataBuffer = Vec::new();
fn from_bytes(bytes: &[u8]) -> Result<Self, Error> {
if bytes.len() > MAX_NINA_BYTE_PARAM_BUFFER_LENGTH {
return Err(ProtocolError::TooManyParameters.into());
}

let mut data_as_bytes: Self::DataBuffer = Vec::new();
data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
Self {
Ok(Self {
length: data_as_bytes.len() as u8,
data: data_as_bytes,
}
})
}

fn data(&self) -> &[u8] {
Expand All @@ -199,6 +205,15 @@ impl NinaConcreteParam for NinaByteParam {
}
}

impl Default for NinaByteParam {
fn default() -> Self {
Self {
length: 0,
data: Vec::new(),
}
}
}

impl NinaConcreteParam for NinaWordParam {
type DataBuffer = Vec<u8, MAX_NINA_WORD_PARAM_BUFFER_LENGTH>;
type LengthAsBytes = [u8; 1];
Expand All @@ -211,13 +226,17 @@ impl NinaConcreteParam for NinaWordParam {
}
}

fn from_bytes(bytes: &[u8]) -> Self {
fn from_bytes(bytes: &[u8]) -> Result<Self, Error> {
if bytes.len() > MAX_NINA_WORD_PARAM_BUFFER_LENGTH {
return Err(ProtocolError::TooManyParameters.into());
}

let mut data_as_bytes: Self::DataBuffer = Vec::new();
data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
Self {
Ok(Self {
length: data_as_bytes.len() as u8,
data: data_as_bytes,
}
})
}

fn data(&self) -> &[u8] {
Expand All @@ -233,6 +252,15 @@ impl NinaConcreteParam for NinaWordParam {
}
}

impl Default for NinaWordParam {
fn default() -> Self {
Self {
length: 0,
data: Vec::new(),
}
}
}

impl NinaConcreteParam for NinaSmallArrayParam {
type DataBuffer = Vec<u8, MAX_NINA_SMALL_ARRAY_PARAM_BUFFER_LENGTH>;
type LengthAsBytes = [u8; 1];
Expand All @@ -245,13 +273,17 @@ impl NinaConcreteParam for NinaSmallArrayParam {
}
}

fn from_bytes(bytes: &[u8]) -> Self {
fn from_bytes(bytes: &[u8]) -> Result<Self, Error> {
if bytes.len() > MAX_NINA_SMALL_ARRAY_PARAM_BUFFER_LENGTH {
return Err(ProtocolError::TooManyParameters.into());
}

let mut data_as_bytes: Self::DataBuffer = Vec::new();
data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
Self {
Ok(Self {
length: data_as_bytes.len() as u8,
data: data_as_bytes,
}
})
}

fn data(&self) -> &[u8] {
Expand All @@ -267,6 +299,15 @@ impl NinaConcreteParam for NinaSmallArrayParam {
}
}

impl Default for NinaSmallArrayParam {
fn default() -> Self {
Self {
length: 0,
data: Vec::new(),
}
}
}

impl NinaConcreteParam for NinaLargeArrayParam {
type DataBuffer = Vec<u8, MAX_NINA_LARGE_ARRAY_PARAM_BUFFER_LENGTH>;
type LengthAsBytes = [u8; 2];
Expand All @@ -279,13 +320,17 @@ impl NinaConcreteParam for NinaLargeArrayParam {
}
}

fn from_bytes(bytes: &[u8]) -> Self {
fn from_bytes(bytes: &[u8]) -> Result<Self, Error> {
if bytes.len() > MAX_NINA_LARGE_ARRAY_PARAM_BUFFER_LENGTH {
return Err(ProtocolError::TooManyParameters.into());
}

let mut data_as_bytes: Self::DataBuffer = Vec::new();
data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
Self {
Ok(Self {
length: data_as_bytes.len() as u16,
data: data_as_bytes,
}
})
}

fn data(&self) -> &[u8] {
Expand All @@ -304,6 +349,15 @@ impl NinaConcreteParam for NinaLargeArrayParam {
}
}

impl Default for NinaLargeArrayParam {
fn default() -> Self {
Self {
length: 0,
data: Vec::new(),
}
}
}

pub(crate) trait ProtocolInterface {
fn init(&mut self);
fn reset<D: DelayMs<u16>>(&mut self, delay: &mut D);
Expand Down Expand Up @@ -356,6 +410,9 @@ pub enum ProtocolError {
InvalidNumberOfParameters,
/// Too many parameters sent over the data bus.
TooManyParameters,
/// Payload is larger than the maximum buffer size allowed for transmission over
/// the data bus.
PayloadTooLarge,
}

impl Format for ProtocolError {
Expand All @@ -365,7 +422,8 @@ impl Format for ProtocolError {
ProtocolError::CommunicationTimeout => write!(fmt, "Communication with ESP32 target timed out."),
ProtocolError::InvalidCommand => write!(fmt, "Encountered an invalid command while communicating with ESP32 target."),
ProtocolError::InvalidNumberOfParameters => write!(fmt, "Encountered an unexpected number of parameters for a NINA command while communicating with ESP32 target."),
ProtocolError::TooManyParameters => write!(fmt, "Encountered too many parameters for a NINA command while communicating with ESP32 target.")
ProtocolError::TooManyParameters => write!(fmt, "Encountered too many parameters for a NINA command while communicating with ESP32 target."),
ProtocolError::PayloadTooLarge => write!(fmt, "The payload is larger than the max buffer size allowed for a NINA parameter while communicating with ESP32 target."),
}
}
}
61 changes: 48 additions & 13 deletions esp32-wroom-rp/src/spi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ where

fn disconnect(&mut self) -> Result<(), Error> {
let dummy_param = NinaByteParam::from_bytes(&[ControlByte::Dummy as u8]);
let operation = Operation::new(NinaCommand::Disconnect).param(dummy_param.into());
let operation =
Operation::new(NinaCommand::Disconnect).param(dummy_param.unwrap_or_default().into());

self.execute(&operation)?;

Expand All @@ -93,9 +94,17 @@ where
// FIXME: refactor Operation so it can take different NinaParam types
let operation = Operation::new(NinaCommand::SetDNSConfig)
// FIXME: first param should be able to be a NinaByteParam:
.param(NinaByteParam::from_bytes(&[1]).into())
.param(NinaSmallArrayParam::from_bytes(&ip1).into())
.param(NinaSmallArrayParam::from_bytes(&ip2.unwrap_or_default()).into());
.param(NinaByteParam::from_bytes(&[1]).unwrap_or_default().into())
.param(
NinaSmallArrayParam::from_bytes(&ip1)
.unwrap_or_default()
.into(),
)
.param(
NinaSmallArrayParam::from_bytes(&ip2.unwrap_or_default())
.unwrap_or_default()
.into(),
);

self.execute(&operation)?;

Expand Down Expand Up @@ -166,10 +175,26 @@ where
) -> Result<(), Error> {
let port_as_bytes = [((port & 0xff00) >> 8) as u8, (port & 0xff) as u8];
let operation = Operation::new(NinaCommand::StartClientTcp)
.param(NinaSmallArrayParam::from_bytes(&ip).into())
.param(NinaWordParam::from_bytes(&port_as_bytes).into())
.param(NinaByteParam::from_bytes(&[socket]).into())
.param(NinaByteParam::from_bytes(&[*mode as u8]).into());
.param(
NinaSmallArrayParam::from_bytes(&ip)
.unwrap_or_default()
.into(),
)
.param(
NinaWordParam::from_bytes(&port_as_bytes)
.unwrap_or_default()
.into(),
)
.param(
NinaByteParam::from_bytes(&[socket])
.unwrap_or_default()
.into(),
)
.param(
NinaByteParam::from_bytes(&[*mode as u8])
.unwrap_or_default()
.into(),
);

self.execute(&operation)?;

Expand All @@ -184,8 +209,11 @@ where
// TODO: passing in TransportMode but not using, for now. It will become a way
// of stopping the right kind of client (e.g. TCP, vs UDP)
fn stop_client_tcp(&mut self, socket: Socket, _mode: &TransportMode) -> Result<(), Error> {
let operation = Operation::new(NinaCommand::StopClientTcp)
.param(NinaByteParam::from_bytes(&[socket]).into());
let operation = Operation::new(NinaCommand::StopClientTcp).param(
NinaByteParam::from_bytes(&[socket])
.unwrap_or_default()
.into(),
);

self.execute(&operation)?;

Expand All @@ -198,8 +226,11 @@ where
}

fn get_client_state_tcp(&mut self, socket: Socket) -> Result<ConnectionState, Error> {
let operation = Operation::new(NinaCommand::GetClientStateTcp)
.param(NinaByteParam::from_bytes(&[socket]).into());
let operation = Operation::new(NinaCommand::GetClientStateTcp).param(
NinaByteParam::from_bytes(&[socket])
.unwrap_or_default()
.into(),
);

self.execute(&operation)?;

Expand All @@ -215,7 +246,11 @@ where
socket: Socket,
) -> Result<[u8; MAX_NINA_RESPONSE_LENGTH], Error> {
let operation = Operation::new(NinaCommand::SendDataTcp)
.param(NinaLargeArrayParam::from_bytes(&[socket]).into())
.param(
NinaLargeArrayParam::from_bytes(&[socket])
.unwrap_or_default()
.into(),
)
.param(NinaLargeArrayParam::new(data).into());

self.execute(&operation)?;
Expand Down