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

add hardware matrix and some fixes for feather-nrf* and doc-gen README #385

Open
wants to merge 1 commit into
base: release
Choose a base branch
from

Conversation

gen2thomas
Copy link

@gen2thomas gen2thomas commented Jan 6, 2024

I have manually collected a new "Feature-Matrix" document, but most likely it would be possible to generate it.

This features are available for "doc-gen":

  • flags to choose the document type for update/creation
  • create machine pages (like before, but will be skipped on errors)
  • adjust "Interfaces" table in mcu documents (like before, but will be skipped on errors)
  • adjust/add "Pins" table in mcu document (like before, but will be skipped on errors)
  • create hardware matrix page as a summary of all mcu pages
  • print a summary at the end of all generation processes with:
    • processed/failed documents
    • failed targets
    • orphaned documents

In addition:

  • add section "Peripheral and Drivers" for hardware-matrix related features
  • adjusted the .gitignore to prevent accidentally add content to PR, created by the test of the page at development time
  • improve README.md for doc-gen
  • fix/add some content in microcontroller pages
  • add pages for "Seeed XIAO ESP32 C3", "Adafruit QT Py ESP32-C3"

Please let me know, what can be improved. E.g. one page each for MCU's which support PWM, BT, WiFi etc. would be more handy?

See #384 for some background

@gen2thomas gen2thomas marked this pull request as draft January 7, 2024 18:36
@gen2thomas gen2thomas force-pushed the feature/add_hardware_matrix branch 13 times, most recently from 7d4ab95 to 3ed77fc Compare January 21, 2024 16:01
@gen2thomas gen2thomas marked this pull request as ready for review January 21, 2024 16:11
@gen2thomas gen2thomas mentioned this pull request Jan 21, 2024
@gen2thomas gen2thomas force-pushed the feature/add_hardware_matrix branch 2 times, most recently from 8461a5b to e99f3ed Compare January 21, 2024 18:36
@aykevl
Copy link
Member

aykevl commented Apr 17, 2024

This PR is really big and difficult to review all at once. Can you maybe split it up a bit?

@aykevl
Copy link
Member

aykevl commented Apr 17, 2024

There are a bunch of different changes that we may or may not want. Putting them all in one big PR makes it near-impossible to be selective about what goes in.

I see you have a bunch of different stuff put together in this PR:

  • Changes to .gitignore. Small, so I don't mind those together with an unrelated PR.
  • Refactored doc-gen. Refactoring like this usually involves a lot of changed lines, so should really be done in a separate commit at least to separate from actual changes.
  • Unrelated changes to imports/main.go (why? They're fine changes, but only add to the size of the PR).
  • A big feature matrix. While this can certainly be useful, I wonder why you're parsing them from the various board .md files instead of generating them at the same time as doc-gen?
  • More extensive README in doc-gen. A good idea, and could be part of a doc-gen refactor or improvement PR.
  • Loads of seemingly unrelated changes to board files (like renaming links)? They may very well be good changes, but aren't really related to doc-gen fixes so should really be in a separate PR.

So basically what I'm asking is to put a bit more work in separating out the changes so that it becomes easier for reviewers to review the PR.

@gen2thomas
Copy link
Author

Hi @aykevl , thanks for (try to) reviewing it and your feedback.

Of course, I can split up the parts, like you suggested. Maybe it is also a good idea to change the target branch to dev for the new PRs.

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.

2 participants