Skip to content

Allow skipping the defmt version check#296

Merged
jonas-schievink merged 2 commits intomainfrom
skip-version-check
Dec 3, 2020
Merged

Allow skipping the defmt version check#296
jonas-schievink merged 2 commits intomainfrom
skip-version-check

Conversation

@jonas-schievink
Copy link
Copy Markdown
Contributor

If we add a --ignore-version-mismatch flag to probe-run, this will make development a bit easier.

Comment thread elf2table/src/lib.rs Outdated

/// Like `parse`, but does not verify that the defmt version in the firmware matches the host.
///
/// Use with caution, this is meant for defmt/probe-run development only.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: I'd put this at the top of the docs so it can't be overseen. plus the --ignore-version-mismatchprobe-run flag should get a "CAUTION: .." doc too?

Comment thread elf2table/src/lib.rs
};

defmt_decoder::check_version(version).map_err(anyhow::Error::msg)?;
if check_version {
Copy link
Copy Markdown
Contributor

@Lotterleben Lotterleben Dec 3, 2020

Choose a reason for hiding this comment

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

personally, I'd rather pull this out of parse_impl() and move it into parse() right before the call to parse_impl(). That way we can avoid having a flag argument* as well as tight coupling between the parsing and defmt consistency check, which imo are two different tasks.

* the function names of parse_ignore_version() and parse() imo already communicate what's happening better than parse(.. , true) does

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Doing the version check requires some amount of processing to get the version, so I'd have to duplicate that into the caller.

tight coupling between the parsing and defmt consistency check, which imo are two different tasks.

There is some amount of intentional coupling between them, for example, we always check that there's only one version in use, and we check for the existence of the .defmt section and the version symbol to determine whether defmt is in use at all, or the linker script is broken.

This PR is only intended to skip the version check, not any of the other consistency checks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hrm , ok then that's maybe one for future refactoring sessions...

@jonas-schievink jonas-schievink merged commit bef79a1 into main Dec 3, 2020
@jonas-schievink jonas-schievink deleted the skip-version-check branch December 3, 2020 12:30
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