tinygo-org / drivers

TinyGo drivers for sensors, displays, wireless adaptors, and other devices that use I2C, SPI, GPIO, ADC, and UART interfaces.
https://tinygo.org
BSD 3-Clause "New" or "Revised" License
587 stars 182 forks source link

first implementation of 1-wire protocol #505

Closed conejoninja closed 1 year ago

conejoninja commented 1 year ago

I would like @dn-kolesnikov to review this PR as it was mainly his work, I just adapted it to the proper repo structure.

Once merge, issue #489 could be closed.

deadprogram commented 1 year ago

Was just looking at this PR. So first of all seems to me that it would be better to structure the files like this:

onewire/onewire.go

And then separately

ds18b20/ds18b20.go

This seems more in keeping with the way things like lora and net are currently.

Future onewire devices (EEPROMs, fuel gauges, etc.) would be added at the root level, like pretty much all of the other drivers are now.

What do you think?

deadprogram commented 1 year ago

Also, to be in fully correct naming it should be 1-wire not onewire.

Due to Go naming rules this would turn into just wire, correct?

This seems appealing:

package somedevice

import "tinygo.org/x/drivers/1-wire"

func (d *Driver) DoSomething() {
    wire.Reset()
    wire.Write(...)
}

What do you think?

conejoninja commented 1 year ago

I just follow @dn-kolesnikov naming.

NOTE: There's a popular Arduino library called Wire that handles I²C communication, https://www.arduino.cc/reference/en/language/functions/communication/wire/ It might be confusing?

deadprogram commented 1 year ago

The name of the package here would be 1-wire In any case, I doubt there would be confusion over that particular name in the code.

Regarding the directory organization, I am not really in favor of the proposed structure. How do we proceed?

conejoninja commented 1 year ago

the structure you proposed seems ok to me and make sense. I'll make the changes and uodate the PR

conejoninja commented 1 year ago

I updated the package as suggested, renamed the folder to 1-wire , and the package to wire (suggested by the IDE was __wire), then, we are forced to import it as

import (
    "machine"
    "time"
    wire "tinygo.org/x/drivers/1-wire"

    "tinygo.org/x/drivers/ds18b20"
)

It's ok for me.

dn-kolesnikov commented 1 year ago

Was just looking at this PR. So first of all seems to me that it would be better to structure the files like this:

onewire/onewire.go

And then separately

ds18b20/ds18b20.go

This seems more in keeping with the way things like lora and net are currently.

Future onewire devices (EEPROMs, fuel gauges, etc.) would be added at the root level, like pretty much all of the other drivers are now.

What do you think?

The Arduino standard library is called OneWire (https://www.arduino.cc/reference/en/libraries/onewire/).

deadprogram commented 1 year ago

@dn-kolesnikov has a good point. We should probably name it onewire.

matveynator commented 1 year ago

Good day ). As each device are using tiny bits of onewire protocol implementation (as I understood - small memory usage wise) it is good that devices are added root level. "onewire" name also seems reasonable... I am also ready to help )

dn-kolesnikov commented 1 year ago

at the moment I am checking the function of searching for multiple devices on the bus. I had to change the Device structure and add a few additional functions. the next commit to my repository will break compatibility with the current versions of the library. the examples will also be rewritten taking into account the new changes.

conejoninja commented 1 year ago

at the moment I am checking the function of searching for multiple devices on the bus. I had to change the Device structure and add a few additional functions. the next commit to my repository will break compatibility with the current versions of the library. the examples will also be rewritten taking into account the new changes.

Feel free to make the PR directly from your repository. I did it because I needed for a FOSDEM talk, but I started to think it will not be merged on time, no need to rush.

deadprogram commented 1 year ago

I was actually getting ready to merge this unless there is some reason I shouldn't?

conejoninja commented 1 year ago

I was actually getting ready to merge this unless there is some reason I shouldn't?

well, @dn-kolesnikov commented the next change will break the compatibility, so I guess it's better to wait

bgould commented 1 year ago

@conejoninja Sounds like @dn-kolesnikov has some new version of this mentioned in #489 ... how should we move forward with this, do you want to merge the changes into your branch or just should @dn-kolesnikov start a new one?

conejoninja commented 1 year ago

I think the conversation is going a bit off-topic. We could merge this and make improvements later.

bxparks commented 1 year ago

I think the conversation is going a bit off-topic. We could merge this and make improvements later.

Sorry about that, I agree.

deadprogram commented 1 year ago

OK based on the last comments I am squash/merging. Thank you very much everyone who worked on this!