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

README tweaks #262

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

README tweaks #262

wants to merge 5 commits into from

Conversation

robamu
Copy link
Contributor

@robamu robamu commented Aug 8, 2021

  1. Additional example for HAL usage with the H7 family using two cores
  2. HTML ID achors for headings added
  3. Some minor formatting improvements and line breaks
  4. Additional usage example for setting the toolchain file

1. Additional example for HAL usage with the H7 family using two cores
2. HTML ID achors for headings added
3. Some minor formatting improvements and line breaks
4. Additional usage example for setting the toolchain file
README.md Outdated
Comment on lines 192 to 199
```cmake
target_link_libraries(${TARGET_NAME} PRIVATE
HAL::STM32::H7::M7::RCC
HAL::STM32::H7::M7::GPIO
HAL::STM32::H7::M7::CORTEX
CMSIS::STM32::H743ZI::M7
STM32::NoSys
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

for all "examples", shouldn't we just link to the example folder ? I mean the readme is here to explain the targets but we have dedicated examples so we should not duplicate the information here.
@Hish15 @robamu what do you think ?

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 think it's okay if the README has a few examples as well to have a general idea how the targets are used. I added that example because it is one that uses two cores.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for delay, we have had less time to dedicate to this repo.
In my experience duplicating too much information in the documentation often ends with documentation not up to date when the code evolves. This is why I think a link to example is better than a copy.

@Hish15 any thoughts about this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll look into it

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 specifically added the dual core example to have at least one simple example for a dual core.
Maybe it would be a good idea to add the H7M7 example to the blinky application?

@Hish15
Copy link
Collaborator

Hish15 commented Jun 12, 2022

Hi,

I personally think that splitting the line like this does not bring any value for a maintainer (The line break changes does not github rendering). Maybe It does, but I cannot see it.
Some cleaning is needed see #270, but none it's actually done here. I think it's better to close this PR without merging it.

@robamu
Copy link
Contributor Author

robamu commented Jun 13, 2022

Do you want to discard all the suggested changes? The anchors are useful to add cross-references inside the README and I think some of the additions increase the usability of the project (e.g. example of hardcoding the toolchain file in CMakeLists.txt to prevent setting the same one all the time).

About the line breaks: You don't see them in github, but I think it's nicer for someone who reads / edits them in a text editor

@Hish15
Copy link
Collaborator

Hish15 commented Jun 13, 2022

You are right on the lines breaks, as I said myself it does not change the generated html file. We can keep them.

The Usage section at the beginning of the file state that you can add multiple vars in different ways, and links to the exemples are provided. CMAKE_TOOLCHAIN_FILE is not in the list of those vars! It's assumed that people will know.
Lets add there instead of at the end.

@robamu
Copy link
Contributor Author

robamu commented Jul 8, 2022

I moved the explanation to the top

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