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.

Sign up for GitHub

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

Jump to bottom

Include SPI master in default build #261

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Include SPI master in default build #261

merged 1 commit into from
Oct 16, 2024

Conversation

jthornblad
Copy link
Contributor

@jthornblad jthornblad commented Aug 30, 2024 •
edited by dehanj
Loading

Description

Suggestion: Always include SPI master in build.

Pro:

  • It's easy to miss to include the SPI master.
  • There are a lot of ugly ifdefs in the design.

Con:

  • We may want to keep it possible to build without SPI and flash access for future releases of Bellatrix-compatible versions. One suggestion, however, is to support that through software with something in the flash saying that flash isn't available and then turning off the future flash code in the firmware.

Type of change

  • Feature (non breaking change which adds functionality)

Submission checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my changes
  • I have tested and verified my changes on target
  • My changes are well written and CI is passing
  • I have squashed my work to relevant commits and rebased on main for linear history
  • I have added a "Co-authored-by: x" if several people contributed, either pair programming or by squashing commits from different authors.
  • I have updated the documentation where relevant (readme, dev.tillitis.se etc.)
  • QEMU is updated to reflect changes

@jthornblad jthornblad requested a review from dehanj August 30, 2024 12:13
@mchack-work
Copy link
Member

Why include it in the default build? What's the rationale?

We want to be able to differ between what kind of product we're building, with or without SPI and flash support among them.

@dehanj
Copy link
Member

dehanj commented Sep 17, 2024

My suggestion is that we keep the ifdefs, but make the default build include the SPI

@dehanj dehanj force-pushed the include_spi_master_in_default_build branch from e625586 to 74e1903 Compare October 16, 2024 08:33
@dehanj
Copy link
Member

dehanj commented Oct 16, 2024

Rebased on main, want to make sure it builds properly before I approve.

@jthornblad @dehanj
Include SPI master in default build
9e57296 
This prevents building without the SPI master, the intention is to use
a different method than if-defs, but it will be introduced at a later
stage.
@dehanj dehanj force-pushed the include_spi_master_in_default_build branch from 74e1903 to 9e57296 Compare October 16, 2024 11:04
dehanj
dehanj approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@dehanj dehanj left a comment

Choose a reason for hiding this comment

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

Updated the hash of the binary. Tested on target. Approved.

@dehanj dehanj merged commit 9e57296 into main Oct 16, 2024
5 checks passed
@dehanj dehanj deleted the include_spi_master_in_default_build branch October 16, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dehanj dehanj dehanj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jthornblad @mchack-work @dehanj