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

device features #90

Merged
merged 1 commit into from
Aug 4, 2019
Merged

device features #90

merged 1 commit into from
Aug 4, 2019

Conversation

burrbull
Copy link
Member

No description provided.

@burrbull
Copy link
Member Author

cc @TheZoq2

@TheZoq2
Copy link
Member

TheZoq2 commented Jul 29, 2019

Replying to @therealprof in the other thread

One of the arguments against it is that we'd have to maintain a database of details even most users will not even know or care about. I'm okay with having features for medium, high, xl and and connectivity lines with low being implied if none of the higher features are selected.

That's a fair point, though I think users would care about it. With this system, you can just easily look at the etching on your chip in order to determine what feature to enable.

However, the feature list is quite large which could be a bit annoying. Do we actually need the information about memory amount, or is density enough? If it is, I think just specifying density might a better option.

@burrbull
Copy link
Member Author

Remove mem and add density or keep both?

@therealprof
Copy link
Member

That's a fair point, though I think users would care about it. With this system, you can just easily look at the etching on your chip in order to determine what feature to enable.

Yeah, maybe... maybe not. Sometimes you might want to use a chip which is sitting inside an enclosure (like an ST-Link Mini clone or a keyboard). Also as done here this gives the wrong impression that it might affect the linker settings at the same time which it will not. And then of course there're the notorious STM32F103C8 which usually is a rebadged STM32F103CB (at least when it comes to the amount of flash available).

I think we really should default to sane settings which will work everywhere and allow the user to set e.g. the "density" to get access to the advanced features.

@TheZoq2
Copy link
Member

TheZoq2 commented Jul 29, 2019

Yea, I think you may be right. So then we want no individual chip flags, and one feature for each density with low as default?

@therealprof
Copy link
Member

@TheZoq2 Unless there's a good reason to have a low feature flag (i.e. there's something removed of the higher levels instead of just added) I'd simply have the status quo be equivalent to low and just add the feature flags for the other levels.

@TheZoq2
Copy link
Member

TheZoq2 commented Jul 29, 2019

Yea, that's probably a better option

@TheZoq2
Copy link
Member

TheZoq2 commented Jul 29, 2019

Shouldn't medium be a subset of high, and high a subset of xl as you did with the memory amount? Or are there things that are removed in higher density devices?

@therealprof
Copy link
Member

I have no idea; was expecting someone who needs those features to chime in. ;)

@burrbull
Copy link
Member Author

RM0008 p.40

@burrbull
Copy link
Member Author

Comments added.

@TheZoq2
Copy link
Member

TheZoq2 commented Jul 29, 2019

RM0008 p.40

Ok, in that case, i'd say this can be merged. Any objections? @therealprof

@therealprof
Copy link
Member

LGTM

@TheZoq2
Copy link
Member

TheZoq2 commented Jul 31, 2019

I had another look at the datasheet, and if i'm reading it correctly, the features should not be subsets of each other. For example, the DAC is available on low density f102 and high density 103, but nothing else
image

This also means that device density is not enough to determine which features are available.

However, since #83 depends on this, perhaps we should stick to just density for now, and deal with subfamilies when we implement something that needs it

@burrbull
Copy link
Member Author

burrbull commented Aug 2, 2019

DAC is available on low density f102 and high density 103

This exception we can check by

#[cfg(any(
    all(feature="stm32f102", not(feature="medium")),
    all(feature="stm32f103", feature="high"),
)]

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 4, 2019

Fair enough, I kind of forgot we have features for the different 10xes 😅. LGTM!

@TheZoq2 TheZoq2 merged commit eff7f30 into stm32-rs:master Aug 4, 2019
@burrbull burrbull deleted the device-features branch August 4, 2019 17:23
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.

3 participants