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

Create and use a ConnectionStatus enum #47

Merged
merged 6 commits into from Nov 25, 2022
Merged

Conversation

dilyn-corner
Copy link
Contributor

@dilyn-corner dilyn-corner commented Nov 17, 2022

Description

This PR attempts to satisfy the requirements of #42 by introducing a new ConnectionStatus enum type.

The variants of ConnectionStatus are intended to align with the official Nina documentation, but slightly renamed to be more informative.

The enum is currently defined in src/wifi.rs as a forward-looking design move.

GitHub Issue: Closes #42

Changes

  • Replace direct byte comparison with ConnectionStatus enum

Testing Strategy

Testing cross/ in the usual way should work as intended. There should be no functional difference in this PR. (I have not run tests).

Concerns

I worked with Jim on this as an intro problem, the changes in src/{lib.rs,spi.rs} are my own guesses :)

At least two variants should be explored further, and they have comments explaining why.

@dilyn-corner dilyn-corner added the enhancement Improves existing functionality or feature label Nov 17, 2022
@dilyn-corner dilyn-corner added this to the 0.3 Release milestone Nov 17, 2022
@dilyn-corner dilyn-corner self-assigned this Nov 17, 2022
esp32-wroom-rp/src/spi.rs Fixed Show fixed Hide fixed
@dilyn-corner
Copy link
Contributor Author

dilyn-corner commented Nov 17, 2022

Officially ran tests which revealed some obvious oversights. I think I did the use ... correctly.

Only one thing missing, in get_conn_status. Is marked as // TODO:.

Warnings were almost all resolved in a different branch, I think? The legitimate complaints here are the lacking docs on the variants for ConnectionStatus.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@dilyn-corner dilyn-corner marked this pull request as draft November 18, 2022 20:16
@dilyn-corner
Copy link
Contributor Author

dilyn-corner commented Nov 19, 2022

Should be good to go now!

Added some extra stuff to wifi.rs because it seemed sane to go in there.

Todos left:

  • document variants
  • rebase

@jhodapp jhodapp changed the title Create and use the ConnectionStatus enum Create and use a ConnectionStatus enum Nov 20, 2022
esp32-wroom-rp/src/wifi.rs Outdated Show resolved Hide resolved
esp32-wroom-rp/src/wifi.rs Outdated Show resolved Hide resolved
dilyn-corner added a commit that referenced this pull request Nov 22, 2022
@dilyn-corner
Copy link
Contributor Author

I would like to draw attention to a3176b0

I removed this comments after a bit of digging.

It looks like WL_IDLE_STATUS is a purely "internal" return value if I'm reading the code correctly. WL_IDLE_STATUS is returned periodically, for instance see WiFiClass::begin(const char* ssid) (and throughout). It seems like this is expected to be returned by us, and not by the Esp32.

As for ApFailed, it looks like it is used in exactly two locations, both here. So it isn't unused as previously commented.

esp32-wroom-rp/src/wifi.rs Fixed Show fixed Hide fixed
Copy link
Contributor

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

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

Looking good, one inline question to think about.

esp32-wroom-rp/src/wifi.rs Show resolved Hide resolved
Copy link
Collaborator

@calebbourg calebbourg left a comment

Choose a reason for hiding this comment

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

This looks great!

I left one thought as well.

esp32-wroom-rp/src/wifi.rs Show resolved Hide resolved
Copy link
Contributor

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

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

Looks very good! Well done and thank you Dilyn!

@dilyn-corner dilyn-corner merged commit f21f7da into main Nov 25, 2022
@dilyn-corner dilyn-corner deleted the connstatus_enum branch November 25, 2022 15:28
dilyn-corner added a commit that referenced this pull request Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves existing functionality or feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Convert get_connection_status() result to be an enum type
3 participants