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

Update esp-hal/README #1228

Merged
merged 8 commits into from
Mar 4, 2024
Merged

Update esp-hal/README #1228

merged 8 commits into from
Mar 4, 2024

Conversation

JurajSadel
Copy link
Contributor

Thank you!

Thank you for your contribution.
Please make sure that your submission includes the following:

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

Removes chip-specific references, briefly describes esp-hal tree, and adds quickstart.
closes #1224

@JurajSadel JurajSadel added the skip-changelog No changelog modification needed label Feb 29, 2024
esp-hal/README.md Outdated Show resolved Hide resolved
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

I have some quite nitpicky comments (sorry!) that I think we should address.

Overall it looks okay but I think we should expand on the features of the HAL, for example, should mention that we support async. I would also link to the the LP hal for LP support core support. I would also link to esp-wifi for wifi/ble support.

esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 29, 2024

Given the README will be on crates.io (forgot about that first) maybe we should "borrow" some ideas and get inspiration from other HALs - e.g. https://crates.io/crates/embassy-stm32

@jessebraham jessebraham mentioned this pull request Mar 1, 2024
4 tasks
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Just a few nitpicks, thanks for taking care of this!

esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
esp-hal/README.md Outdated Show resolved Hide resolved
@SergioGasquez
Copy link
Member

SergioGasquez commented Mar 4, 2024

Regarding format, might be worth to try following the style guide that we wrote for the book: https://github.com/esp-rs/book/blob/main/rust-doc-style-guide.md. I've just used the vale config created from those rules and here is the output:

❯ vale esp-hal/README.md --config /home/sergio/Documents/Espressif/esp-vale-config/.vale.ini

 esp-hal/README.md
 1:3     warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 8:5     warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 15:14   suggestion  Consider removing 'closely'.    Espressif-latest.Adverbs                    
 15:242  error       Use 'Wi-Fi' instead of 'WiFi'.  Espressif-latest.TermsSingleCorrectSpelling 
 22:4    warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 47:5    warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 62:1    suggestion  Try to keep sentences short (<  Microsoft.SentenceLength                    
                     30 words).   

✖ 1 error, 4 warnings and 2 suggestions in 1 file.

Most of the things are not relevant Id say

@MabezDev
Copy link
Member

MabezDev commented Mar 4, 2024

Regarding format, might be worth to try following the style guide that we wrote for the book: https://github.com/esp-rs/book/blob/main/rust-doc-style-guide.md. I've just used the vale config created from those rules and here is the output:

❯ vale esp-hal/README.md --config /home/sergio/Documents/Espressif/esp-vale-config/.vale.ini

 esp-hal/README.md
 1:3     warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 8:5     warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 15:14   suggestion  Consider removing 'closely'.    Espressif-latest.Adverbs                    
 15:242  error       Use 'Wi-Fi' instead of 'WiFi'.  Espressif-latest.TermsSingleCorrectSpelling 
 22:4    warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 47:5    warning     Use heading-style               Espressif-latest.Headings                   
                     capitalization. Capitalize                                                  
                     articles and prepositions of                                                
                     4 letters and less only at the                                              
                     beginning and end of heading                                                
                     title. See the rule's code for                                              
                     other exceptions.                                                           
 62:1    suggestion  Try to keep sentences short (<  Microsoft.SentenceLength                    
                     30 words).   

Most of the things are not relevant Id say

Oh that's really nice! Do you know if we can run this in CI 👀. Maybe that's not worth the effort though.

@SergioGasquez
Copy link
Member

Oh that's really nice! Do you know if we can run this in CI 👀. Maybe that's not worth the effort though.

That was the idea, at least for the books/trainings, but so far it's not possible as the repo with vale rules is under GitLab and its closed-source. I've had some conversations about how to use it on CI, but I haven't heard any update regarding this in the last months, I'll try to get an update.

@jessebraham
Copy link
Member

jessebraham commented Mar 4, 2024

I think prose linting our READMEs in CI is probably barbaric overkill, but can we move this to a discussion or something instead please, so the conversation doesn't disappear with this PR.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 4, 2024

Would be good if the usage paragraph would link to the (not merged yet) README for the examples ( #1237 ) - not sure if we then want or should have the command to run examples duplicated or not

JurajSadel and others added 7 commits March 4, 2024 15:48
Co-authored-by: Jesse Braham <jessebraham@users.noreply.github.com>
Co-authored-by: Jesse Braham <jessebraham@users.noreply.github.com>
Co-authored-by: Scott Mabin <scott@mabez.dev>
* Mostly rewrite `esp-hal/README.md` (oops :) )

* Fix a typo
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

I've had my way with this file now, so LGTM 😁

@jessebraham jessebraham merged commit 0e0035c into esp-rs:main Mar 4, 2024
1 check passed
@JurajSadel JurajSadel deleted the docs branch April 10, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The esp-hal/README needs updating
6 participants