Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wifinina: connect improvements #561

Merged
merged 3 commits into from
May 18, 2023
Merged

Conversation

deadprogram
Copy link
Member

This PR adds a few improvements for connecting the WiFiNINA.

It corrects the timing used for the RESET pins when calling Configure() to match other libraries.

It also adds a new ResetIsHigh property to allow changing the behavior of the RESET pin for boards like the Arduino MKR 1010, which correct #386

Lastly, it makes some changes to the various examples to make a few retry attempts when connecting to the WiFi access point, which appears to solve most of the issues when connecting especially right after flashing.

Note that this PR depends on #560 which needs to be merged first, and then this PR rebased.

@deadprogram deadprogram requested review from bgould and sago35 and removed request for bgould May 3, 2023 04:53
@deadprogram
Copy link
Member Author

Now that I have been testing #537 I am going to close this PR. The ResetIsHigh can be added in a separate PR once #537 makes it in.

@deadprogram deadprogram closed this May 4, 2023
@deadprogram deadprogram reopened this May 9, 2023
@deadprogram
Copy link
Member Author

Reopening since #537 will not merge until TinyGo 0.29

…vice reset

Signed-off-by: deadprogram <ron@hybridgroup.com>
…ctions

Signed-off-by: deadprogram <ron@hybridgroup.com>
…r boards like the Arduino MKR 1010

Signed-off-by: deadprogram <ron@hybridgroup.com>
@deadprogram deadprogram force-pushed the wifinina-connect-improvements branch from ae3a69a to 1a618b6 Compare May 14, 2023 21:17
@deadprogram
Copy link
Member Author

Rebased against dev with #560 included. Please review! 😸

@scottfeldman
Copy link
Contributor

scottfeldman commented May 17, 2023

I've picked up the ResetIsHigh changes from this PR and copied them to #537.

All the other timing changes have already been picked up in #537.

// should be High or Low (the default). Set this to true
// before calling Configure() for boards such as the Arduino MKR 1010,
// where the reset signal needs to go high instead of low.
ResetIsHigh bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This does look to me as a hack/workaround.
Shall we not have RESET_HIGH and RESET_LOW pins then instead?
Alternatively RESET_HIGH and RESET for backwards compatibility.
Then users can choose which pin to assign and which leave as "nopin".
And logic in Configure() then decides which pin to use (can panic / error when both defined or none)

Copy link
Contributor

Choose a reason for hiding this comment

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

Another alternative: control behaviour of RESET pin with a build tag.
Arduino MKR 1010 target could have, say, wifinina_reset_high build tag. Next, wifinina driver has an extra file with this tag here that sets an internal flag for such behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both sound more complicated somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Second alternative is more complicated to implement perhaps, but easier and transparent to use.
To me it feels the reset pin behaviour is board/hardware specific and weird to have to think about it in the application code.

The first approach, I like it less myself, just brainstormed how can we do it differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have drivers that have different behaviour for different build tags already, see https://github.com/tinygo-org/drivers/tree/release/ws2812

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but they do more than just swap high/low for a specific pin. Adding a build tag for that seems like overkill, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

The change I made seemed to me like the "simplest thing that could possibly work" to address the swap of setting high/low. Adding more code/logic just seemed like a lot extra for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

To me it feels the reset pin behaviour is board/hardware specific and weird to have to think about it in the application code.

It is a single line you need to add if you are using one of only a couple of specific boards. Hence why I tried to make the least intrusive change possible.

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.

After discussions in slack and in the light of the big upcoming refactoring in this package (netdev), it is no point to dwell on minor API changes now.

[seal of approval]

@deadprogram
Copy link
Member Author

Merging to keep the progress/bug fixes moving along. Thanks everyone!

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.

3 participants