Skip to content

Conversation

@GabeMillikan
Copy link
Contributor

@GabeMillikan GabeMillikan commented Nov 7, 2025

In the coming weeks, we'll be implementing abstractions for many different peripherals. We'll all be working in parallel in the same directory, so I'd like to set up that directory now so that we don't all have overlapping changes later.

I also added a "demo abstraction" for USART just to show how to setup CMake correctly, and what file structure we'd like to follow for each abstraction.

@GabeMillikan GabeMillikan marked this pull request as ready for review November 7, 2025 02:42
Copy link
Contributor

@dchansen06 dchansen06 left a comment

Choose a reason for hiding this comment

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

Please keep G4HELLO as is, this can be added to G4PERTESTING though

@dchansen06 dchansen06 added Documentation Improvements or additions to documentation Enhancement New feature or request CMake Anything related to or dealing with CMake 2 PRIORITY Important and a priority, but less than URGENT Peripheral Related to or involving a peripheral including abstractions labels Nov 7, 2025
@dchansen06 dchansen06 added this to the GR26 milestone Nov 7, 2025
@dchansen06
Copy link
Contributor

Also please update the PR title, thanks!

@dchansen06 dchansen06 marked this pull request as draft November 7, 2025 05:29
@GabeMillikan GabeMillikan changed the title prepare Lib/Inhouse directory for peripheral abstractions create Lib/Peripherals/ to house upcoming peripheral abstractions Nov 7, 2025
@GabeMillikan GabeMillikan marked this pull request as ready for review November 7, 2025 23:23
Copy link
Contributor

@dchansen06 dchansen06 left a comment

Choose a reason for hiding this comment

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

To keep within scope of create Lib/Peripherals/ to house upcoming peripheral abstractions please remove the modifications to G4PERTESTING, these can be part of a separate PR as part of your implementation work

@dchansen06
Copy link
Contributor

Fyi I am debugging why it says merging is blocked itia

image

Copy link
Contributor

@dchansen06 dchansen06 left a comment

Choose a reason for hiding this comment

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

Re-approving after ruleset update

@GabeMillikan
Copy link
Contributor Author

Re-approving after ruleset update

Still no luck for me, but I'm not sure if I'm supposed to be able to merge into main anyway
image

@dchansen06
Copy link
Contributor

Still no luck for me, but I'm not sure if I'm supposed to be able to merge into main anyway

After PR approval you should be able to as well, but I am also blocked at the moment (though I can admin override that is not a fix)

dchansen06
dchansen06 previously approved these changes Nov 10, 2025
Copy link
Contributor

@dchansen06 dchansen06 left a comment

Choose a reason for hiding this comment

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

Final approval after ruleset configuration

@dchansen06
Copy link
Contributor

(I am turning off the rules to see if that fixes it and then turning them back on one-by-one, please do not merge)

@dchansen06
Copy link
Contributor

Should be good, can you please try merging it?

dchansen06 added a commit that referenced this pull request Nov 10, 2025
# Code Owners

## Problem and Scope
Current roles were overly restrictive (see #107) and blocked merges

## Description
Require approval from the firmware lead in the `CODEOWNERS` file before
merging

## Gotchas and Limitations
Must keep the file up to date, anyone with root can override it of
course

## Testing

- [ ] HOOTL testing
- [ ] HITL testing
- [x] Human tested

### Testing Details
To be seen, may auto-request reviews but that is life

## Larger Impact
None as all PRs are already reviewed by firmware lead

## Additional Context and Ticket
See the issues in #107

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@dchansen06 dchansen06 force-pushed the gabe.peripherals-skeleton branch from 8a61a8a to bfa8588 Compare November 10, 2025 01:26
Copy link
Contributor

@dchansen06 dchansen06 left a comment

Choose a reason for hiding this comment

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

Update with rebase for #111 and #110

@dchansen06
Copy link
Contributor

@GabeMillikan Please try squash merging when you are ready, thanks!

@GabeMillikan GabeMillikan merged commit 4ff2735 into main Nov 10, 2025
1 check passed
@GabeMillikan GabeMillikan deleted the gabe.peripherals-skeleton branch November 10, 2025 05:05
@GabeMillikan
Copy link
Contributor Author

@GabeMillikan Please try squash merging when you are ready, thanks!

@dchansen06 Worked! Thanks

@dchansen06
Copy link
Contributor

@dchansen06 Worked! Thanks

Awesome! @GabeMillikan Can you please try (updating the branch of) and confirming that #55 is not mergable at this point since it has not been approved? (If it is possible please do not actually merge though lol)

@GabeMillikan
Copy link
Contributor Author

@dchansen06 Worked! Thanks

Awesome! @GabeMillikan Can you please try (updating the branch of) and confirming that #55 is not mergable at this point since it has not been approved? (If it is possible please do not actually merge though lol)

@dchansen06 will do 🫡

@GabeMillikan
Copy link
Contributor Author

@dchansen06 Worked! Thanks

Awesome! @GabeMillikan Can you please try (updating the branch of) and confirming that #55 is not mergable at this point since it has not been approved? (If it is possible please do not actually merge though lol)

looks like the new ruleset is working correctly 👍
{20EBE753-826E-41A1-8365-12A63D844BF1}

@dchansen06
Copy link
Contributor

Awesome thanks! That took way too long to fix lol (I was the one who broke it of course)

dchansen06 added a commit that referenced this pull request Dec 4, 2025
# Code Owners

## Problem and Scope
Current roles were overly restrictive (see #107) and blocked merges

## Description
Require approval from the firmware lead in the `CODEOWNERS` file before
merging

## Gotchas and Limitations
Must keep the file up to date, anyone with root can override it of
course

## Testing

- [ ] HOOTL testing
- [ ] HITL testing
- [x] Human tested

### Testing Details
To be seen, may auto-request reviews but that is life

## Larger Impact
None as all PRs are already reviewed by firmware lead

## Additional Context and Ticket
See the issues in #107

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 PRIORITY Important and a priority, but less than URGENT CMake Anything related to or dealing with CMake Documentation Improvements or additions to documentation Enhancement New feature or request Peripheral Related to or involving a peripheral including abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants