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 README.md to account for new makefile setup #57

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

ReFil
Copy link
Collaborator

@ReFil ReFil commented Nov 16, 2022

No description provided.

@ReFil ReFil linked an issue Nov 16, 2022 that may be closed by this pull request
Copy link
Contributor

@allanwind allanwind left a comment

Choose a reason for hiding this comment

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

make and make all is the same thing with this config file I suggest you just use the former. The make install feature was merged so we should mention that in step 2 as an option on Linux (and welcome suitable changes on other platforms) and maybe note official process which was to put both boards into update mode (or whatever it was called). The write-up you did for me when it was required was really good.

For completeness, let's mention make clean to remove accumulated firmware and removal of the containers.

Sorry, I should have updated the readme.txt. The file was being actively modifying when I made those changes initially.

Happy to do another review.

Copy link
Contributor

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ReFil ReFil force-pushed the 56-instructions-in-readme-fail branch 2 times, most recently from f26ffa9 to 07bde13 Compare November 17, 2022 13:25
@ReFil
Copy link
Collaborator Author

ReFil commented Nov 17, 2022

@allanwind The make install stuff wasn't merged afaik, but i've made the other changes you suggested including mentioning the clean up stuff. I also added firmware install instructions linking to the kinesis QSG

@ReFil ReFil requested a review from allanwind November 18, 2022 17:28
@ReFil ReFil force-pushed the 56-instructions-in-readme-fail branch from 07bde13 to 54abe0b Compare November 18, 2022 18:09
Copy link
Contributor

@allanwind allanwind left a comment

Choose a reason for hiding this comment

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

Looks great and you can ship it as is.

The document would look simpler if you just have two sections (Github, localhost) and list the steps sequentially. You can mark the setup steps with "(once)" but I think it would be clear implied.

"Note: Either Podman or Docker is required, Podman is preferred if both are present. If compiling on Windows use WSL2 and docker [Docker Setup Guide](https://docs.docker.com/desktop
/windows/wsl/)." is a little verbose. What about calling "Install container run-time" then as bullets for the alternatives: + On Windows use WSL2 and docker Docker Setup Guide." Otherwise Docker or Podman (latter is preferred if both are installed). Is "latest" clear? It was for me.

I.e. along these lines:

Github

  1. (once) Fork this repo.
  2. (once) Enable GitHub Actions on your fork (how?)
  3. Push a commit to trigger the build.
  4. Download the artifact (where?)
  5. Quick Start Guide, p. 8 tells you how to install the two new firmware files.

Local

  1. (once) Install container run-time
    • On non-Windows use Docker or Podman (latter is preferred if both are available).
    • On Windows use WSL2 and docker Docker Setup Guide.
  2. make
  3. Quick Start Guide, p. 8 tells you how to install the two latest firmware files (ls firmware | tail -2).

Maybe even one list (I have to write it out to tell if it's better or worse):

  1. Setup your environment and build firmware. You can do so either on github or on your localhost:
    • github
      1. (once) Fork this repo.
      2. (once) Enable GitHub Actions on your fork (how?)
      3. Push a commit to trigger the build.
      4. Download the artifact (where?)
    • localhost
      1. (once) Install container run-time
        • On non-Windows use Docker or Podman (latter is preferred if both are available).
        • On Windows use WSL2 and docker Docker Setup Guide.
      2. make
      3. The latest two firmware files (ls firmware | tail -2) is the ones you just build
  2. Quick Start Guide, p. 8 tells you how to install the firmware files.

@ReFil
Copy link
Collaborator Author

ReFil commented Nov 18, 2022

That's a good idea, i'll do that, documentation isn't really my strong suit so i appreciate the advice :)

@allanwind
Copy link
Contributor

I find it hard, too :-)

@ReFil ReFil force-pushed the 56-instructions-in-readme-fail branch from d39a0a2 to 49fb545 Compare November 18, 2022 22:32
@ReFil ReFil requested a review from allanwind November 21, 2022 11:45
Copy link
Contributor

@allanwind allanwind left a comment

Choose a reason for hiding this comment

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

All the sections make it look more complicated than it is. Did you see the write up using a single enumerated list? In either case feel free to ship it.

@ReFil
Copy link
Collaborator Author

ReFil commented Nov 21, 2022

Thanks for the advice. I wanted to give enough information to make it easier, although I can see how it can look quite complicated, it's a hard balance to walk. I think I'll leave it as is

@ReFil ReFil merged commit 0fb8e58 into V2.0 Nov 21, 2022
tricktux pushed a commit to tricktux/Adv360-Pro-ZMK that referenced this pull request Feb 10, 2024
…nstructions-in-readme-fail

Update README.md to account for new makefile setup
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.

Instructions in README fail
3 participants