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

Added all timers for all variants as described by CubeMX #133

Merged
merged 2 commits into from
Nov 20, 2019

Conversation

cs2dsb
Copy link
Contributor

@cs2dsb cs2dsb commented Nov 9, 2019

I've added all the timers according to the CubeMX database with the correct device & density flags.

The only one's I've had to comment out are 9, 10, 12, 13 and 14 on some devices because fields for these are missing from the underlying pac. The code is there to enable them when stm32-rs fixes up those SVDs.

This might be a minor breaking change as previously some of the devices had timers enabled that aren't actually available across the whole range - the fix is to add the appropriate density feature. I've added some info about this to the README so hopefully people will find this without a problem.

There's also some minor formatting stuff mostly from rustfmt in case you are wondering about the whitespace changes in pwm_input.

I should also draw your attention to the fact I added #![allow(unused_unsafe)] (with a comment) to lib.rs to hide the extremely numerous warnings that were coming up because some pac have safe fields incorrectly identified as unsafe. I think adding #![allow(unused_unsafe)] to the crate is fine and preffererable to it being harder to read the compiler output but I'm happy to be persuaded otherwise if you disagree.

Copy link
Contributor

@geomatsi geomatsi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

src/timer.rs Outdated Show resolved Hide resolved
src/rcc.rs Outdated Show resolved Hide resolved
@cs2dsb cs2dsb force-pushed the add_all_timers branch 2 times, most recently from ce95872 to ee1729e Compare November 10, 2019 19:08
@cs2dsb
Copy link
Contributor Author

cs2dsb commented Nov 10, 2019

I left the piddling little rustfmt changes from random files out but added the pwm_input formatting fix back in as a separate commit because it took me half an hour to tweak the indentation and I feel like it's worth keeping ;)

src/timer.rs Outdated Show resolved Hide resolved
src/timer.rs Outdated Show resolved Hide resolved
@cs2dsb cs2dsb force-pushed the add_all_timers branch 2 times, most recently from a946265 to 7b0ffc6 Compare November 14, 2019 12:38
@cs2dsb
Copy link
Contributor Author

cs2dsb commented Nov 14, 2019

Rebased latest changes from master.

Is there anything else I need to address to get this merged? I'm going to have a go at implementing the various PWM input & output features for the advanced timers and want to make sure that PR is easy to merge too.

Thanks
Daniel

TIM8: (tim8, pclk2_tim, dbg_tim8_stop),
}

//TODO: restore these timers once stm32-rs has been updated
Copy link
Member

Choose a reason for hiding this comment

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

stm32-rs 0.9 released and we now use it or you mean something other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That refers to the missing dbg_tim(9 to 14)_stop fields on dbgmcu. They haven't been implemented in stm32-rs yet.

Copy link
Member

@TheZoq2 TheZoq2 left a comment

Choose a reason for hiding this comment

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

Apart from the #[allow(unused_unsafe)] comment, this looks good to me

// Some pac crates have fields specified in such a way that they are safe and other
// have them unsafe (likely an SVD error that needs patching). Unsafe blocks for
// one device cause warnings for the safe devices. This disables that warning.
#![allow(unused_unsafe)]
Copy link
Member

Choose a reason for hiding this comment

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

I think this error should have been fixed by the latest PAC release, could you check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are still instances of mixed safe & unsafe registers. Examples:
stm32f100::tim1::aar::AAR_W::bits is safe
stm32f100::tim6::aar::AAR_W::bits is unsafe

I think the original comment still stands for now.

Copy link
Member

@TheZoq2 TheZoq2 Nov 20, 2019

Choose a reason for hiding this comment

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

Depending on how many such warnings there are, I would prefer having the #[allow] thing on the lines/functions where it happens. That way we don't miss actually useful warnings

Although, we could do that in a separate PR

@burrbull
Copy link
Member

I think it can be merged after conflicts to be resolved.

@cs2dsb
Copy link
Contributor Author

cs2dsb commented Nov 17, 2019

Updated with latest master.
Note I had to modify the new start_master (#99) part of the macro because the general purpose timers 9 to 14 don't support master mode.

@TheZoq2 TheZoq2 merged commit e4e4233 into stm32-rs:master Nov 20, 2019
@cs2dsb cs2dsb deleted the add_all_timers branch November 21, 2019 20:07
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.

4 participants