Skip to content

Conversation

@soypat
Copy link
Contributor

@soypat soypat commented Apr 7, 2025

recently was inspired to start working on protips for people designing drivers.

Still a WIP.

Would love a review and suggestions from driver developer extraordinaires @aykevl @deadprogram @ysoldak @sago35

@amken3d
Copy link

amken3d commented Apr 8, 2025

Nice. This would hav helped me when I was writing the TMC drivers. I like the practical aspect of the write up. You should do something similar for tips and tricks for more tinygo topics.

@ysoldak
Copy link
Contributor

ysoldak commented Apr 8, 2025

I like "To abstract machine.Pin one can define type PinOutput func(level bool) and type PinInput func() bool functional interfaces."
Another alternative is for us to have drivers.Pin, but that's a separate discussion and I don't think we come to any decision yet? Also, do we expect pins be pre-configured or a driver's Config method to set them up?
Anyway, the core advise is to avoid machine package, that's good 👍
We can always update docs later when we decide on pins.

@ysoldak
Copy link
Contributor

ysoldak commented Apr 8, 2025

@soypat
Copy link
Contributor Author

soypat commented Apr 8, 2025

@ysoldak I've applied your suggestions- thank you so much!

- Store HAL required for peripheral operation here such as communication buses (I2C, SPI drivers) and pins
- **Avoid**: using TinyGo `machine` package constructs in your driver (read as *don't import `"machine"`*). Users of the Go language (not only TinyGo) also consume the drivers package and they can't compile a program that uses the `"machine"` package. To abstract `machine.Pin` one can define `type PinOutput func(level bool)` and `type PinInput func() bool` functional interfaces.

- Define a `Config` struct for peripherals with many configuration options. See example of a Config struct in action: https://github.com/soypat/lora/blob/main/lora.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would point to a driver under the tinygo-org repo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! I've deleted that link and added a list of compliant drivers below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, sadly all the drivers are mine :(. Be great to start working on making other existing drivers compliant so we can expand the list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we have resolved this question yet, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which question?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find other examples which do a good job to sticking to the guidlines except for the ones added in 7905de7

Copy link
Contributor

@ysoldak ysoldak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

@deadprogram deadprogram changed the base branch from release to dev April 12, 2025 09:30
@soypat
Copy link
Contributor Author

soypat commented Apr 13, 2025

Found a couple typos and in the heat of it added a concurrency section which I felt was missing based on my experiences programming and using async drivers in TinyGo.

@soypat
Copy link
Contributor Author

soypat commented Apr 17, 2025

Last call for comments before merging! Will wait until Monday :)

@soypat soypat merged commit ebdd023 into dev Apr 22, 2025
4 checks passed
@soypat soypat deleted the driver-design branch April 22, 2025 02:04
@soypat
Copy link
Contributor Author

soypat commented Apr 22, 2025

Mergerino the pull requesterino

@soypat
Copy link
Contributor Author

soypat commented Apr 22, 2025

Thank you @ysoldak, @conejoninja for the feedback <3

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.

6 participants