Skip to content

Added support for block reading and writing - #127

Merged
CurlyMoo merged 5 commits into
wiringX:masterfrom
Satharus:master
Sep 2, 2024
Merged

CurlyMoo merged 5 commits into
wiringX:masterfrom
Satharus:master

Conversation

@Satharus

Copy link
Copy Markdown
Contributor

Hello there,

Thanks for the great work with this library.

I added three functions to support reading and writing blocks with a specified size to the I2C device. This can particularly be useful if a slave device replies back with a DWORD (e.g., one of its registers is 32-bit). For writing, I added two versions, one that implicitly sends the size as the first byte to the slave device and one that doesn't.

I tested these changes on the Milk-V Duo S using their fork of WiringX and it works fine. All I need to do is replace /usr/lib/libwiringx.so on the Milk-V with the new binary and include it in the sdk toolchain as well.

I also replaced __time_t and __suseconds_t with time_t and suseconds_t, respectively. This allows it to be cross-compiled for rv64 with musl.

Thanks!

Added three functions to support reading and writing blocks with a specified size to the I2C device. For writing, I added two versions, one that implicitly sends the size as the first byte to the slave device and one that doesn't.
@CurlyMoo

Copy link
Copy Markdown
Contributor

using their fork of WiringX and it works fine

First of all, why are there forks? The whole idea of this library is of course that we have one single library that covers many different devices at once. Luckily i didn't have to confince Radxa to contribute to this repo directly. Hopefully, you can confince Milk-V to do the same.

Comment thread src/i2c-dev.c
Comment thread src/wiringx.c
@CurlyMoo

Copy link
Copy Markdown
Contributor

The PR looks good. @nascs can you maybe confirm that this doesn't affect at least one other device besides Milk-V ones?

@Satharus

Copy link
Copy Markdown
Contributor Author

Hi @CurlyMoo,

I've removed the newlines. Thanks for checking the PR.

As to why Milk-V have their own fork, I honestly have no idea... I originally tried using this library but of course, it would give an error when I call wiringXSetup("milkv_duos", NULL) as the device isn't implemented here.

I think maybe someone could add their changes here? I am not sure what exact changes they made but I am worried that they may have made some breaking ones. I also think it can't be a single merge at a point in time, their future changes have to be included here as well, what do you think?

@CurlyMoo

CurlyMoo commented Aug 29, 2024 •

Copy link
Copy Markdown
Contributor

It would be great if we at least could have the conversation. The more brands supporting wiringX by default the better.

If they are not willing at first you could take the lead in the discussion. By letting me know where the conflicts are if you would backport their changes here.

@Satharus

Copy link
Copy Markdown
Contributor Author

It would be great if we at least could have the conversation

Just to understand the context, have you tried reaching out to them before? I tried to make a similar PR on their repo around 2 weeks ago and haven't had any response.

By letting me know where the conflicts are if you would backport their changes here.

Can probably try and do that at some point, I'll let you know how it goes. In that case, I'll open a different PR for it. The only reason I referred to Milk-V in this one is because it was the board I tested my changes on as part of a project I was working on.

@CurlyMoo

Copy link
Copy Markdown
Contributor

Just to understand the context, have you tried reaching out to them before?

No, until your PR i didn't know about the existance of that fork.

@Satharus

Copy link
Copy Markdown
Contributor Author

No, until your PR i didn't know about the existance of that fork.

Sounds good, thanks. I'll try and see if they respond to my PR and also take a look at what changes they made and how/if they could be merged here as well.

@CurlyMoo

Copy link
Copy Markdown
Contributor

I now see that i merged a Milk-V PR 5 months ago so their was cooperation. Is there more or was that it?

@CurlyMoo

Copy link
Copy Markdown
Contributor

@willian-milk @carbonfix @MilkVYuntian what are your thoughts and input on this?

@CurlyMoo

Copy link
Copy Markdown
Contributor

Milkv themselves suggest this approach in regard to the time and seconds definitions:

diff --git a/src/wiringx.c b/src/wiringx.c
index 034674a..4171a75 100644
--- a/src/wiringx.c
+++ b/src/wiringx.c
@@ -113,6 +113,9 @@ static struct spi_t spi[2] = {
        } while(0)
 #endif

+typedef time_t __time_t;
+typedef suseconds_t __suseconds_t;
+
 /* Both the delayMicroseconds and the delayMicrosecondsHard
    are taken from wiringPi */
 static void delayMicrosecondsHard(unsigned int howLong) {

Which indeed is a better approach. Is there a C MACRO that we can use to either use the Milkv desired definition or the default one?

@carbonfix

Copy link
Copy Markdown
Contributor

Thank you guys! Milk-V also aims to use the official wiringX code and submit our modifications to the upstream wiringX repo. We have already done relevant work before. The reason why there are separate forks is that some functions are still being verified with users, and some codes are not standardized enough. We will make some modifications and submit them to the upstream wiringX.

@nascs

nascs commented Aug 30, 2024

Copy link
Copy Markdown
Contributor

The PR looks good. @nascs can you maybe confirm that this doesn't affect at least one other device besides Milk-V ones?

Ok, I'll test it.

@CurlyMoo

CurlyMoo commented Aug 30, 2024 •

Copy link
Copy Markdown
Contributor

@nascs When @Satharus changes the PR to only make these changes in case of a Milk-V compilation, nothing changes for the other platforms. So there is no more need to test.

@CurlyMoo

Copy link
Copy Markdown
Contributor

@carbonfix It would be great if you could soon move the discussion of the standardization of code to the wiringX repo when applicable.

@carbonfix

Copy link
Copy Markdown
Contributor

@carbonfix It would be great if you could soon move the discussion of the standardization of code to the wiringX repo when applicable.

We will complete this work as soon as possible, thanks!

@Satharus

Copy link
Copy Markdown
Contributor Author

@nascs When @Satharus changes the PR to only make these changes in case of a Milk-V compilation, nothing changes for the other platforms. So there is no more need to test.

Why do I have to do that? I think merging Milk-V's fork is a different task from merging my functions.

The functions I implemented here should work on any other device as well. I was just stating where I tested them. Hence why it would be great if someone could kindly test my changes.

@CurlyMoo

Copy link
Copy Markdown
Contributor

Why do I have to do that?

You are confusing the two converstations here. My question was if you could adopt the typedef approach for time_t and suseconds_t as Milk-V suggested. But with the addition that it only uses those definitons in case of a Milk-V or MUSL compilation.

@Satharus

Copy link
Copy Markdown
Contributor Author

My question was if you could adopt the typedef approach for time_t and suseconds_t as Milk-V suggested. But with the addition that it only uses those definitons in case of a Milk-V or MUSL compilation.

Ahh, I got what you mean now. I'll look into it and change the PR.

@Satharus

Satharus commented Sep 1, 2024

Copy link
Copy Markdown
Contributor Author

Hi @CurlyMoo

I got around to adding the changes. I added conditional macros for RISC-V only.

Thanks

Comment thread src/wiringx.c Outdated
Comment thread src/wiringx.c Outdated
Comment thread src/wiringx.h Outdated
Comment thread src/wiringx.h Outdated
@Satharus

Satharus commented Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

@CurlyMoo Removed them, thanks

@CurlyMoo
CurlyMoo merged commit 35ec63c into wiringX:master Sep 2, 2024
@CurlyMoo

CurlyMoo commented Sep 2, 2024

Copy link
Copy Markdown
Contributor

Thanks!

@Satharus

Satharus commented Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

You're more than welcome, happy to help!

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.

4 participants