IO implementation for embedded-io #101

Artur-Romaniuk · 2023-12-23T20:21:26Z

Related to #100

newAM

Looks good, thanks for adding the feature, just some structural changes I would make.

newAM · 2023-12-25T21:57:32Z

src/eh1/io.rs

This should go under src/io.rs since embedded-io is versioned independently of embedded-hal.

@dbrgn If you could give your opinion on this?

Yeah, I think the suggestion makes sense.

Cargo.toml

Artur-Romaniuk

Will apply suggested changes.

Cargo.toml

src/eh1/io.rs

Artur-Romaniuk · 2024-01-14T09:59:04Z

Rebased on main. Would love to have some feedback.

newAM

I had some nitpicks with the documentation links but I just did the changes and pushed them since that's easier.

Looks good to me overall. We're going to have unfortunate conflicts with the other PR that touches delays, but that's unavoidable. Will wait for dbrgn's feedback on this too.

src/common.rs

Artur-Romaniuk · 2024-02-03T11:08:16Z

This PR went stale for a while now. @dbrgn could take a look?

dbrgn · 2024-02-03T20:35:41Z

@Artur-Romaniuk because this PR was created on top of the delay branch (#104), I wanted to wait until that branch is ready and merged (and left feedback there) to make review easier.

If you want to make PRs easier to review, try to keep them small and focussed. Having a lot of practice with git rebase (especially interactive rebase) helps a lot with that.

Artur-Romaniuk · 2024-02-04T18:52:25Z

@Artur-Romaniuk because this PR was created on top of the delay branch (#104), I wanted to wait until that branch is ready and merged (and left feedback there) to make review easier.

If you want to make PRs easier to review, try to keep them small and focussed. Having a lot of practice with git rebase (especially interactive rebase) helps a lot with that.

That was a fuckup...
Must have rebased this branch on wrong origin/main. Force pushed rebased version with all unecessary commits dropped.

dbrgn

I left a few last comments. The rest is looking great, I think once the comments are addressed this can be merged.

dbrgn · 2024-05-16T20:14:00Z

src/eh1/io.rs

Yeah, I think the suggestion makes sense.

src/eh1/io.rs

Cargo.toml

dbrgn · 2024-05-16T20:21:18Z

src/common.rs

+/// Optional Data type allows for storage of arbitrary state data in mocks.
+/// Mock data can be accessed via appropriate getter/setter methods, i.e. `mock_data()` and `set_mock_data()`.


Suggested change

/// Optional Data type allows for storage of arbitrary state data in mocks.

/// Mock data can be accessed via appropriate getter/setter methods, i.e. `mock_data()` and `set_mock_data()`.

/// The `mock_data` field allows for storage of arbitrary state data in mocks.

/// Mock data can be accessed via appropriate getter/setter methods, i.e. `mock_data()` and `set_mock_data()`.

dbrgn · 2024-05-16T20:21:42Z

src/common.rs

 #[derive(Debug, Clone)]
-pub struct Generic<T: Clone + Debug + PartialEq> {
+pub struct Generic<T: Clone + Debug + PartialEq, Data = ()> {


I like the T-prefix convention for type parameters. Can you rename Data to TData?

dbrgn · 2024-05-16T20:28:24Z

src/eh1/io.rs

+//! // Finalizing expectations
+//! io.done();
+//!
+//! // optional async


Suggested change

//! // optional async

//! // Async is supported as well, if the `embedded-hal-async` feature is enabled

dbrgn · 2024-05-16T20:32:28Z

src/eh1/io.rs

+    pub fn write(expected: Vec<u8>) -> Transaction {
+        Transaction {
+            expected_mode: Mode::Write,
+            expected_data: expected,


Suggested change

pub fn write(expected: Vec<u8>) -> Transaction {

Transaction {

expected_mode: Mode::Write,

expected_data: expected,

pub fn write(expected_data: Vec<u8>) -> Transaction {

Transaction {

expected_mode: Mode::Write,

expected_data,

dbrgn · 2024-05-16T20:36:57Z

src/eh1/io.rs

+            panic!("response longer than read buffer for io::read");
+        }
+
+        let len = std::cmp::min(buffer.len(), transaction.response.len());


Since we already assert above, that the response length can't be larger than the buffer length, can't we just use the response length here?

Suggested change

let len = std::cmp::min(buffer.len(), transaction.response.len());

let len = transaction.response.len();

dbrgn · 2024-05-16T20:38:32Z

src/eh1/io.rs

+        let transaction = self.next().expect("no expectation for io::seek call");
+
+        if let Mode::Seek(expected_pos) = transaction.expected_mode {
+            assert_eq!(expected_pos, pos, "io::seek unexpected mode");


Suggested change

assert_eq!(expected_pos, pos, "io::seek unexpected mode");

assert_eq!(expected_pos, pos, "io::seek unexpected pos");

dbrgn · 2024-05-16T20:48:00Z

src/eh1/io.rs

+        let response_vec = transaction.response;
+        self.set_mock_data(Some(response_vec));


Since this variable is only used once, I think it can be inlined.

Suggested change

let response_vec = transaction.response;

self.set_mock_data(Some(response_vec));

self.set_mock_data(Some(transaction.response));

victorbnl · 2024-07-24T12:29:23Z

Any chance this gets fixed and merged in a near future?

dbrgn · 2024-09-13T20:13:24Z

@Artur-Romaniuk do you have plans to finish this PR, or would you like for someone else to take over?

newAM requested changes Dec 25, 2023

View reviewed changes

Artur-Romaniuk commented Dec 25, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Cargo.toml Show resolved Hide resolved

dbrgn reviewed Dec 26, 2023

View reviewed changes

src/eh1/io.rs Show resolved Hide resolved

src/eh1/io.rs Outdated Show resolved Hide resolved

src/eh1/io.rs Outdated Show resolved Hide resolved

src/eh1/io.rs Outdated Show resolved Hide resolved

src/eh1/io.rs Outdated Show resolved Hide resolved

src/eh1/io.rs Show resolved Hide resolved

Artur-Romaniuk added a commit to Artur-Romaniuk/embedded-hal-mock that referenced this pull request Dec 26, 2023

Applied changes from review for embedded-io dbrgn#101

43094f9

Artur-Romaniuk requested a review from newAM January 10, 2024 08:46

Artur-Romaniuk added a commit to Artur-Romaniuk/embedded-hal-mock that referenced this pull request Jan 14, 2024

Applied changes from review for embedded-io dbrgn#101

df5ebb7

Artur-Romaniuk force-pushed the io-mock branch from 43094f9 to df5ebb7 Compare January 14, 2024 09:36

newAM approved these changes Jan 14, 2024

View reviewed changes

src/common.rs Show resolved Hide resolved

Artur-Romaniuk added a commit to Artur-Romaniuk/embedded-hal-mock that referenced this pull request Feb 4, 2024

Applied changes from review for embedded-io dbrgn#101

73ee134

Artur-Romaniuk force-pushed the io-mock branch from ecf014e to 2705618 Compare February 4, 2024 18:27

Artur-Romaniuk added 4 commits February 4, 2024 19:48

Added mocks for embedded-io and -io-async

b533588

Applied changes from review for embedded-io dbrgn#101

dbddc6f

Updated docs for IO

cfac2f2

added mock data description in common.rs

09a582a

Artur-Romaniuk force-pushed the io-mock branch from 2ec3594 to 09a582a Compare February 4, 2024 18:49

dbrgn requested changes May 16, 2024

View reviewed changes

dbrgn added the waiting-for-author label May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IO implementation for embedded-io #101

IO implementation for embedded-io #101

Artur-Romaniuk commented Dec 23, 2023

newAM left a comment

newAM Dec 25, 2023

Artur-Romaniuk Dec 26, 2023

dbrgn May 16, 2024

Artur-Romaniuk left a comment

Artur-Romaniuk commented Jan 14, 2024

newAM left a comment

Artur-Romaniuk commented Feb 3, 2024

dbrgn commented Feb 3, 2024

Artur-Romaniuk commented Feb 4, 2024

dbrgn left a comment

dbrgn May 16, 2024

dbrgn May 16, 2024

dbrgn May 16, 2024 •

edited

Loading

dbrgn May 16, 2024

dbrgn May 16, 2024

dbrgn May 16, 2024

dbrgn May 16, 2024

dbrgn May 16, 2024 •

edited

Loading

victorbnl commented Jul 24, 2024

dbrgn commented Sep 13, 2024

		/// Optional Data type allows for storage of arbitrary state data in mocks.
		/// Mock data can be accessed via appropriate getter/setter methods, i.e. `mock_data()` and `set_mock_data()`.

	//! // optional async
	//! // Async is supported as well, if the `embedded-hal-async` feature is enabled

	let len = std::cmp::min(buffer.len(), transaction.response.len());
	let len = transaction.response.len();

	assert_eq!(expected_pos, pos, "io::seek unexpected mode");
	assert_eq!(expected_pos, pos, "io::seek unexpected pos");

		let response_vec = transaction.response;
		self.set_mock_data(Some(response_vec));

	let response_vec = transaction.response;
	self.set_mock_data(Some(response_vec));
	self.set_mock_data(Some(transaction.response));

IO implementation for embedded-io #101

Are you sure you want to change the base?

IO implementation for embedded-io #101

Conversation

Artur-Romaniuk commented Dec 23, 2023

newAM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artur-Romaniuk left a comment

Choose a reason for hiding this comment

Artur-Romaniuk commented Jan 14, 2024

newAM left a comment

Choose a reason for hiding this comment

Artur-Romaniuk commented Feb 3, 2024

dbrgn commented Feb 3, 2024

Artur-Romaniuk commented Feb 4, 2024

dbrgn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbrgn May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbrgn May 16, 2024 • edited Loading

Choose a reason for hiding this comment

victorbnl commented Jul 24, 2024

dbrgn commented Sep 13, 2024

dbrgn May 16, 2024 •

edited

Loading

dbrgn May 16, 2024 •

edited

Loading