IO implementation for embedded-io by Artur-Romaniuk · Pull Request #101 · rust-embedded/embedded-hal-mock

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));

marie-bnl · 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?

esden · 2025-03-09T21:31:51Z

Would like to bump this issue. Any chance to get embedded-io support added, maybe even async?

This applies _most_ of the final comments from the review on rust-embedded#101. I phrased one of the changes slightly differently.

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 rust-embedded#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 rust-embedded#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 rust-embedded#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 rust-embedded#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

tshakah pushed a commit to seagen-io/embedded-hal-mock that referenced this pull request Mar 19, 2025

Applied changes from review for embedded-io rust-embedded#101

160d71e

tshakah added a commit to seagen-io/embedded-hal-mock that referenced this pull request Mar 19, 2025

Fixes from review

30992b7

This applies _most_ of the final comments from the review on rust-embedded#101. I phrased one of the changes slightly differently.

tshakah mentioned this pull request Mar 19, 2025

IO implementation for embedded-io, rebased and updated #126

Open

tshakah added a commit to seagen-io/embedded-hal-mock that referenced this pull request Mar 19, 2025

Fixes from review

f42b26c

This applies _most_ of the final comments from the review on rust-embedded#101. I phrased one of the changes slightly differently.

tshakah added a commit to seagen-io/embedded-hal-mock that referenced this pull request Mar 19, 2025

Fixes from review

a71d0c3

This applies _most_ of the final comments from the review on rust-embedded#101. I phrased one of the changes slightly differently.

		/// 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));

Comments

Conversation

Artur-Romaniuk commented Dec 23, 2023

Uh oh!

newAM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Artur-Romaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Artur-Romaniuk commented Jan 14, 2024

Uh oh!

newAM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Artur-Romaniuk commented Feb 3, 2024

Uh oh!

dbrgn commented Feb 3, 2024

Uh oh!

Artur-Romaniuk commented Feb 4, 2024

Uh oh!

dbrgn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbrgn May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbrgn May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marie-bnl commented Jul 24, 2024

Uh oh!

dbrgn commented Sep 13, 2024

Uh oh!

esden commented Mar 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dbrgn May 16, 2024 •

edited

Loading

dbrgn May 16, 2024 •

edited

Loading