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

[refactoring] Cleanup for option bytes and flash settings #941

Merged
merged 14 commits into from
Apr 22, 2020

Conversation

grevaillot
Copy link
Collaborator

@grevaillot grevaillot commented Apr 20, 2020

hi,

Some more work on flash handling, mostly around implementing and reflactoring register access and flash locking helpers. option write/read device specific has been refactored based on the main flash familly, and f0 support primitives have been added.

a second pr will follow later to support then.

@grevaillot
Copy link
Collaborator Author

ok, so, this was supposed to be a draft pr but i validated too fast... I've got a couple of fixed to push but some review would be welcome

@grevaillot grevaillot changed the title To merge/option2 option (and flash) refactoring / cleanup. Apr 20, 2020
src/common.c Show resolved Hide resolved
src/common.c Outdated Show resolved Hide resolved
@slyshykO
Copy link
Collaborator

@grevaillot I think, use an assert() call in library code not right.

We have closed issue about it. #634

@grevaillot
Copy link
Collaborator Author

updated patchset with fixes to above comments

@grevaillot I think, use an assert() call in library code not right.

We have closed issue about it. #634

hmm, i might have looked too fast then, i thought release builds where defining NDEBUG - discarding assert statements ? i'll check.

@slyshykO
Copy link
Collaborator

hmm, i might have looked too fast then, i thought release builds where defining NDEBUG - discarding assert statements

Agree.
But take in consideration that we directly apply NDEBUG only for msvc toolset.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Apr 21, 2020

I'd like to suggest to do feature- and code testing before opening a PR in future, because once opened this keeps updating the travis build system with loads of step by step build tests which actually should be done locally by running ./travis.sh . The travis CI build history would be much easier to follow, if only builds would be listed that are about to merge or have merged. This also applies to drafts BTW. Please consider this for future work.

Edit: The only option I found in the settings is to completely disable PR pushes which obviously is not what we want, as the are useful in general.

@grevaillot
Copy link
Collaborator Author

ah. sorry for the ci stuff. code and feature is pretty much done except the assert stuff.

Agree.
But take in consideration that we directly apply NDEBUG only for msvc toolset.

Release builds on linux too apparently, I could not find where this was setup, but build is run with -DNDEBUG. I'd be happy to understand why :)

@slyshykO
Copy link
Collaborator

slyshykO commented Apr 21, 2020

I could not find where this was setup, but build is run with -DNDEBUG.

Usually CMAKE_C_FLAGS_RELEASE cmake variable contains -DNBEBUG option, but it may be changed by command-line options or by another way. This is not mandatory that cmake by default should provide NDEBUG. So, my opinion is, or we will set NDEBUD directly in our cmake project, or we shouldn't use asserts in library code.

@Nightwalker-87
Copy link
Member

@grevaillot: It's ok. No problem yet. BTW: There is also the possibility to discuss within an issue with references to a personal active dev-branch. This should be the same user experience but could be a solution.

give l0/l1 some love - l0/l1 are a bit specific regarding keys,
bits, and lock registers.

Also make handling of f0/f1 explicit: ultimate goal is to have some
sanity check and clear codepath in that code in case some new target
is added - and that the whole code uses these helpers.

some work should be done to refactor a bit the flash register mapping
based on family, but not yet.

lo lock
1) no need to unlock option flash to read it.

"The option byte are always read-accessible and write-protected by default."

current device specific option read code now only differs from generic read code
by reading option bytes in the flash option byte register instead of option base.

2) f4 and f2 have similar registers : use the same write code, with flash/option lock
helpers. add flash busy wait and relock at end of access.

3) add f446 option info to chip id
use generic unlock for l4, rename l496 to l4x, add wait flash busy,
complete chip id for l476 (and other)
merge l0/l1, code is the same except flash base, add wait states and relock
should only happen currently after adding a new flash family.
now that all flash family methods are aligned, refactor
main method to avoid duplicated lock/unlock, add info traces, and
map methods based on familly instead of a small subset of selected
devices.

nb1: f0/f1 support is still missing

nb2: option read/write is still conditioned by presence of option_base
and option_size in chipid.c, but adding more chips should only be
a matter of adding these two informations.
@grevaillot
Copy link
Collaborator Author

okay, i removed asserts and fixed a regression, lets review merge these bits now if it's ok with you.

@Nightwalker-87 Nightwalker-87 changed the title option (and flash) refactoring / cleanup. [refactoring] Cleanup for option bytes and flash settings Apr 22, 2020
@Nightwalker-87 Nightwalker-87 merged commit 027c4b4 into stlink-org:develop Apr 22, 2020
@Nightwalker-87
Copy link
Member

Thanks for your contribution!

@stlink-org stlink-org locked as resolved and limited conversation to collaborators Jun 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants