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

Fix doctests in attiny-hal #616

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

innermatrix
Copy link
Contributor

This PR fixes all existing doctests in attiny-hal and adds cargo test --doc to CI for attiny-hal. Stacked on #605.

@innermatrix innermatrix force-pushed the attiny-fix-doctests branch 3 times, most recently from 928daa8 to 4df103f Compare December 30, 2024 13:18
@stappersg
Copy link
Contributor

On Sun, Dec 29, 2024 at 05:08:53PM -0800, Iris Artin wrote:

@innermatrix pushed 1 commit.

928daa8 Fixed attiny doctests

Hey, #617 has also "Fixed attiny doctests"

@innermatrix
Copy link
Contributor Author

innermatrix commented Dec 30, 2024

On Sun, Dec 29, 2024 at 05:08:53PM -0800, Iris Artin wrote:

@innermatrix pushed 1 commit.
928daa8 Fixed attiny doctests

Hey, #617 has also "Fixed attiny doctests"

Yeah I have several PRs that are all against main but they are stacked on top of each other so the later ones include all the commits from the previous ones. If you'd like me to update them so that each is against the topic branch of the previous one, I can do that.

(I wish GH had better support for stacked PRs, but it doesn't, so here we are; I am happy to use whatever workflow works best for you.)

@stappersg
Copy link
Contributor

stappersg commented Dec 30, 2024 via email

@stappersg
Copy link
Contributor

stappersg commented Dec 30, 2024 via email

@innermatrix
Copy link
Contributor Author

innermatrix commented Dec 30, 2024

tldr: Thank you and @Rahix for all your great work; I think my contributions would be valuable to the community, so I would love to align with you and @Rahix — in whatever way you find supportive — on my proposed direction. I think a good place to start is for you two to give feedback in #605 about my proposal to have one module to describe each MCU (for example, attiny85.rs). Everything else I think we can figure out downstream.

A longer version and responses to some of your specific points:

Best answer to "If you'd like" is asking "How would I like it myself".

Thank you for saying that — I very much agree with that as an organizing principle. On this particular question, I have seen two different approaches, and I have used both successfully, which is why I picked one and am letting you guide me as to whether you prefer the other. (The two approaches being: 1. every stacked PR is against main and reviewers know to select a subset of commits when reviewing or 2. every stacked PR is against another PR's topic branch and maintainers know in which order they need to be merged.)

Unfortunately, approach 2 (which is kinder to reviewers) is not supported by GH for cross-fork PRs. The only way I could make that work is if I was able to create PRs directly in the main repo.

So for now I am just going to hold off to avoid creating more burden for you; please see below on what I think is the most valuable to review right now. And I do apologize if I overwhelmed you with too many PRs and not enough information about where to focus your limited reviewing energy.

What I remember, there were messages like: @innermatrix: What if refactor some parts? @stappersg: Come back with something to review. What I can't remember to have seen is any message from Rahix about the refactoring proposals.

You remember correctly!

I think there are two main issues we are running into here. One is that it was not clear to me whether you or @Rahix were the project lead at this time, so I wasn't sure how much involvement from @Rahix to expect; thank you for clarifying that.

The other issue is that the changes I am proposing are fairly foundational, and as a result, it's not easy to present a small thing for review. (Breaking it up into smaller pieces actually ends up being more work because of the way that the different parts currently interact with each other. See below for more on this, though.

But hey, you are allowed to keep your fork.

If that's how it turns out, that's okay, but I would much prefer to create something that we can agree to merge back in.

As you saw, I started some additional work that improves documentation and reliability (by adding compile-only doctests for all MCUs to CI). I also have the beginnings of support for ATtiny 0-series and 1-series MCUs that I plan to submit in a future PR. Finally, I have been thinking about what it would take to run unit tests against real AVR hardware, and I plan to bring that up for discussion when I have some coherent thoughts.

I think that those contributions would likely be valuable to the community, and so I would much prefer to integrate them with the great work you are already doing, instead of creating a long-term divergence. I would be happy to work with you and @Rahix on figuring out what form those contributions should take, just let me know what works best for you.

My goal here is to give back to the community in whatever form is the most supportive of you two. What I ask in return is guidance about the direction you want to take with this, and ongoing open communication.

To make that easier, let me ask a specific question:

When you have the time, could you or @Rahix please look at my proposed structural changes in attiny-hal (which submitted in #605) and tell me whether you think this is a good direction. The main structural change I am proposing in that PR is to refactor the crate to have a separate module for each MCU, which acts as a description of that specific device (as opposed to the current code, which is organized by peripheral). You can see the big picture structure in https://github.com/innermatrix/avr-hal/tree/attiny-hal-additive-features/mcu/attiny-hal/src and the per-MCU structure in any of the files in there (such as https://github.com/innermatrix/avr-hal/tree/attiny-hal-additive-features/mcu/attiny-hal/src/attiny85.rs)

We can (and should) have a discussion later about lesser questions (naming, style, implementation details, etc), but the main thing that I am proposing to do, and that everything else I would like to contribute is built on, is that structural change: one module to describe each MCU. So that's the main question that I would like to align on early.

Once again, thank you!

@stappersg
Copy link
Contributor

( see #623 )

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