firmata: addition of support for firmata protocol by imle · Pull Request #20 · periph/devices

imle · 2021-10-12T01:46:11Z

I wanted to open this early as it is a significant change and I would really like some input before I get much deeper into this implementation. There are parts of this missing and I have not implemented some of the features yet. This is a work in progress.

googlebot · 2021-10-12T01:48:08Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

imle · 2021-10-12T01:49:45Z

@googlebot I signed it!

googlebot · 2021-10-12T01:49:54Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2021-10-12T01:52:11Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

maruel · 2021-10-13T14:23:16Z

bmxx80/bmxx80.go

 	"periph.io/x/conn/v3/spi"
 )

+// I2CAddr i2c default address.


Can you make a separate PR for this and the file below? They are unrelated to firmata.

Didn't mean for that to make it in, will move to another PR.

maruel · 2021-10-13T14:23:42Z

Apologies for the delay. Thanks I'll try to look at it ASAP.

maruel

Thanks for the PR! That's a lot of functionality, that's really cool.

maruel · 2021-10-16T22:41:36Z

firmata/analog_mapping.go

@@ -0,0 +1,43 @@
+package firmata


For every files: please add copyright.

maruel · 2021-10-16T22:41:48Z

firmata/analog_mapping.go

+	"fmt"
+)
+
+type AnalogMappingResponse struct {


For every public symbols: Please document or make private.

maruel · 2021-10-16T22:42:04Z

firmata/analog_mapping.go

+}
+
+func ParseAnalogMappingResponse(data []byte) AnalogMappingResponse {
+	var response = AnalogMappingResponse{


For most var uses in this PR: var is unnecessary.

maruel · 2021-10-16T22:42:32Z

firmata/analog_mapping.go

+
+func ParseAnalogMappingResponse(data []byte) AnalogMappingResponse {
+	var response = AnalogMappingResponse{
+		AnalogPinToDigital: []uint8{},


This is unnecessary.

maruel · 2021-10-16T22:43:18Z

firmata/analog_mapping.go

+	return response
+}
+
+func (a AnalogMappingResponse) String() string {


I recommend a pointer receiver otherwise it makes a copy.

maruel · 2021-10-16T22:46:35Z

firmata/client.go

+	sysExListenerChannels map[SysExCmd]chan []byte
+
+	i2cListeners map[uint8]chan I2CPacket
+	i2cMU        sync.Mutex


Put the mutex above what it protects, not below.

maruel · 2021-10-16T22:47:05Z

firmata/client.go

+	Close() error
+}
+
+type Client struct {


Can you either export the interface or the struct but not both? I generally prefer the struct unless there's a clear reason like to make a fake/mock easier to use in practice.

maruel · 2021-10-16T22:48:00Z

firmata/client.go

+	go func() {
+		err := c.responseWatcher()
+		if err != nil {
+			panic(err)


Can you make this not be a panic?

maruel · 2021-10-16T22:50:17Z

firmata/util.go

@@ -0,0 +1,114 @@
+package firmata


name this file "conv.go", it's more meaningful than util.go.

maruel · 2021-10-16T22:50:40Z

pca9685/pins.go


 func (p *pin) WaitForEdge(timeout time.Duration) bool {
-	return false
+	return gpio.INVALID.WaitForEdge(timeout)


Is that intended?

maruel · 2021-11-14T01:19:02Z

What I would recommend is to start with only one subsystem, let's say i2c, and go from there. The PR is fairly large and will make the back and forth more difficult.

imle added 4 commits October 4, 2021 02:35

Working firmata and onewire

12356d3

add i2c implementation

050bc7a

updates and testing of implementations

7b71c74

cleanup of pin function names

b9ac491

maruel reviewed Oct 13, 2021

View reviewed changes

maruel reviewed Oct 16, 2021

View reviewed changes

Conversation

imle commented Oct 12, 2021

Uh oh!

googlebot commented Oct 12, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

imle commented Oct 12, 2021

Uh oh!

googlebot commented Oct 12, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Oct 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maruel commented Oct 13, 2021

Uh oh!

maruel 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maruel commented Nov 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants