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

Minor updates #139

Merged
merged 7 commits into from
Jan 8, 2024
Merged

Minor updates #139

merged 7 commits into from
Jan 8, 2024

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Jan 3, 2024

  • Fix format in a few files
  • Update deps
  • verify.sh now exits on errors

Supersedes #135

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 4, 2024

I think it will cause problems with esp-wifi since 0.1.1 needs the older HAL. We could make it a git-dependency and patch a lot of the dependencies like in https://github.com/esp-rs/esp-wifi/blob/12477b3b5fd3b3b17727a0471b624eb5b0578f6e/Cargo.toml#L59-L65

Really hope our dependencies get a release and we are able to release esp-wifi 0.2.0 since it's blocking a few things

@MabezDev
Copy link
Member

MabezDev commented Jan 4, 2024

Hopefully we will have a enet release soon: embassy-rs/embassy#2401 then we can bump esp-wifi and merge this.

@MabezDev
Copy link
Member

MabezDev commented Jan 4, 2024

embassy-net 0.3 is now available on crates.io so I think we can get things moving here!

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 4, 2024

embassy-net 0.3 is now available on crates.io so I think we can get things moving here!

Awesome! I will update + release esp-wifi tomorrow

@@ -6,21 +6,31 @@ edition = "2021"
license = "MIT OR Apache-2.0"

[dependencies]
hal = { package = "{{ mcu }}-hal", version = "{{ hal_version }}" }
{{ mcu }}-hal = "{{ hal_version }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason we don't want to alias the HAL dependency anymore?

Copy link
Member

@jessebraham jessebraham Jan 5, 2024

Choose a reason for hiding this comment

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

What was the reason we did it in the first place? I always found it weird and needless, every time I generated a template I changed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ... interesting. I found it to be easier to copy code around this way ... however, I don't have a hard preference. Was just curious - and also once we did the split, we will just have esp_hal or something

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel too strongly about this, so feel free to revert if anybody else does.

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont have any hard preference, tbh

Copy link

Choose a reason for hiding this comment

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

The main idea behind alias is to increase portability of the code. Using alias makes it easier to have common app code for multiple targets.

Copy link
Member

Choose a reason for hiding this comment

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

The next release will unify the hal, meaning all hals will be exposed under esp_hal, just like esp_idf_hal supports all target in one.

Cargo.toml Outdated
smoltcp = { version = "0.11.0", default-features=false, features = ["proto-igmp", "proto-ipv4", "socket-tcp", "socket-icmp", "socket-udp", "medium-ethernet", "proto-dhcpv4", "socket-raw", "socket-dhcpv4"] }
embedded-svc = { version = "0.26.4", default-features = false, features = [] }
embedded-io = "0.6.1"
heapless = { version = "0.8.0", default-features = false }
Copy link
Contributor

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 should bump this since it's mostly used for embedded-svc which is still on 0.7+

Copy link
Contributor

Choose a reason for hiding this comment

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

oh sorry - we shouldn't downgrade embedded-io but heapless: https://crates.io/crates/embedded-svc/0.26.4/dependencies

Reason is, it's used in esp-wifi's (through embedded-svc) public API

Copy link
Contributor

Choose a reason for hiding this comment

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

downgrading smoltcp is probably good since for some reason we forgot to bump it in esp-wifi 0.2.0 🤦‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

I think now, if I understood it correctly, dependencies should be fine.

@SergioGasquez SergioGasquez force-pushed the fix/updates branch 2 times, most recently from a58686c to 26766bb Compare January 8, 2024 09:17
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks

@SergioGasquez SergioGasquez merged commit bf31770 into esp-rs:main Jan 8, 2024
7 checks passed
@SergioGasquez SergioGasquez deleted the fix/updates branch January 8, 2024 10:31
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.

5 participants