Add support for reading and writing to the Backup Data Register (#83). #86

mjepronk · 2019-07-16T08:49:13Z

This is my first attempt at fixing issue #83.

TheZoq2 · 2019-07-16T09:24:01Z

Thanks for the PR, it looks good to me, and most of my comments are "bikeshedding"

I'm not sure how I feel about panic! when indexing out of bouds as panics can be a pain to debug on embedded platforms. Since reg 0 doesn't exist, those errors would probably be pretty common as well.
On the other hand, data registers are something that never changes, so it should be fairly simple to catch the panic in a debugger.

Another option might be to ignore the names of the registers and make the indexes 0 based in the API. As far as I know, no other systems in the processor depends on data in these registers so where we put "1" won't make a difference.

Whichever option we go with, it would be nice with a more clear comment saying which range of registers are available.

Since I'm bikeshedding already, I think I would prefer read_data_register over read_bkp_dr. Most of the functions in the rest of the crate avoids abbreviated names.

Finally, could you add a line to CHANGELOG.md saying that this feature has been implemented?

src/backup_domain.rs

mjepronk · 2019-07-16T09:30:42Z

No problem, bikeshedding is fine.
I'll change the function names to read_data_register and write_data_register.
I'll add a line to CHANGELOG.md.
What do the others think of the register: u8 argument, zero or one based index?
What do you think of the array based functions?

src/backup_domain.rs

therealprof · 2019-07-16T09:49:25Z

What do the others think of the register: u8 argument, zero or one based index?

0 based. I'm just not so sure about the u8, maybe it would be better to make it usize for compatibility with general indexing operations.

src/backup_domain.rs

therealprof · 2019-07-16T14:26:02Z

Hm, just noticed that travis failed for STM32F100 and STM32F101; are the SVD files broken and/or do we need to need to feature gate the backup domain stuff for STM32F103 and higher?

burrbull · 2019-07-16T14:30:49Z

Hm, just noticed that travis failed for STM32F100 and STM32F101; are the SVD files broken and/or do we need to need to feature gate the backup domain stuff for STM32F103 and higher?

stm32-rs have regression. STM32F103 and other have different types.
I've fixed it in stm32-rs/stm32-rs#256

mjepronk · 2019-07-16T14:35:07Z

I've found that the BKP_DR11 to BKP_DR42 registers do not work (writing a value to them and reading afterwards gives back 0). I already found that they used a different naming convention in the STM32F1 crate. Does anybody know more about this?

burrbull · 2019-07-16T14:44:32Z

I've found that the BKP_DR11 to BKP_DR42 registers do not work (writing a value to them and reading afterwards gives back 0). I already found that they used a different naming convention in the STM32F1 crate. Does anybody know more about this?

It can be problem of incorrect BKP register address:

burrbull · 2019-07-16T14:55:59Z

stm32-rs/stm32-rs@58f8163
should fix problem

burrbull · 2019-07-16T14:59:32Z

Related issue in old repo:
japaric/stm32f103xx#18

mjepronk · 2019-07-16T15:34:25Z

@burrbull Thanks, I have tried your change and I saw that you've renamed bkp_dr11 - bkp_dr42 to dr11 - dr42, great! But they still don't work. Also, dr1 does not seem to work either...

I'll update this PR with my latest code. The code I'm using to test is:

        let mut buf_w: [u16; 42] = [0; 42];
        for (i, d) in buf_w.iter_mut().enumerate() {
            *d = (i + 1) as u16;
        }
        backup_domain.write_data_register_array(&buf_w, 0);

        let mut buf_r: [u16; 42] = [0; 42];
        backup_domain.read_data_register_array(&mut buf_r, 0);

        for (i, (w, r)) in buf_w.iter().zip(buf_r.iter()).enumerate() {
            if w != r {
                hprintln!("{}: ERR w: {} ≠ r: {}", i, w, r).unwrap();
            }
        }

Update: I've pushed the code to this PR. Please note that it requires the latest master from stm32-rs repository to build successfully (because of name changes from bkp_dr11 to dr11 etc).
Update 2: Improved my testing code...

burrbull · 2019-07-16T15:45:13Z

With this change dr will be array.
So you don't need match.
Access with dr[i] and bkp_dr[j]
where i = 0..9 j = 0..31

mjepronk · 2019-07-16T15:50:36Z

Aah, I'm sorry for the trouble, thanks. Why do we split into dr and bkp_dr? Should we reflect this divide in the API we're defining here too?

burrbull · 2019-07-16T15:52:31Z

    pub fn read_data_register(&self, register: usize) -> u16 {
        match register {
            1..10  => read_drx!(self, dr, register-1),
            11..42 =>  read_drx!(self, bkp_dr, register-11)
             _=> unreachable!()
        }
   }

burrbull · 2019-07-16T15:54:53Z

Aah, I'm sorry for the trouble, thanks. Why do we split into dr and bkp_dr? Should we reflect this divide in the API we're defining here too?

Because svd2rust still not support deriving for registers:
Waiting for:
rust-embedded/svd2rust#256

dr's splitted on 2 regions, so these 2 arrays must have different names anyway.

src/backup_domain.rs

burrbull · 2019-07-16T16:59:46Z

src/backup_domain.rs

+macro_rules! write_drx {
+    ($self:ident, $drx:ident, $idx:expr, $new:expr) => {{
+        unsafe {
+            $self._regs.$drx[$idx].write(|w| w.bits($new.into()));


$self._regs.$drx[$idx].write(|w| w.d().bits($new));
And and unsafe is not needed.

Apparently, I still need unsafe. I've added the .d() part.

src/backup_domain.rs

mjepronk · 2019-07-17T13:08:46Z

OK, so the following points are open:

We should decide if we want to prevent people with low- or medium density STM32's of using the registers DR11-DR42 at compile time. Right now, reading always returns 0 and writing is a noop on these devices.
The PR for the stm32-rs repository from burrbull needs to be merged (and released?) before it makes sense to merge this code.
There is still code in stm32-rs repository that requires unsafe to write to either bkp or bkp_dr. See burrbull's review comments.

Let me know if there is still something I can do!

burrbull · 2019-07-17T13:28:42Z

I think we should split each function on low/high variant, where first one will operate on dr and second feature gated function will operate on bkp_dr.

TheZoq2 · 2019-07-17T21:22:59Z

We should decide if we want to prevent people with low- or medium density STM32's of using the registers DR11-DR42 at compile time. Right now, reading always returns 0 and writing is a noop on these devices.

I'm not a fan of runtime checks, so I'd prefer a feature gate. If we do do runtime checks, i'd prefer some sort of error over returning valid looking data like 0

I think we should split each function on low/high variant, where first one will operate on dr and second feature gated function will operate on bkp_dr.

I like this solution

mjepronk · 2019-07-17T22:14:51Z

OK, I separated the functions in _low and _high.

I dropped the array / buffer functions, because I think it would introduce to much functions for just the backup register. But I think this code is very straightforward to implement by the library user. Also they are not always practical (for example if I want to store a u32 in the backup data register, I would have to go from a u32 to an array of 2 u16's anyway). If you want them back, shout out!

I'm not sure how to do the feature gate:

low/medium or high/xl density?
stm32f101, stm32f102, ..? or
high-backup-register?

Sorry, for the lots of back- and forward...

src/backup_domain.rs

mjepronk · 2019-07-27T16:15:49Z

What do we do with the feature gate?
When will a new version of stm32f1 be released?
What else is in the way of merging this?

burrbull · 2019-07-27T16:28:23Z

You can add #[cfg(feature="stm32f107")].

@adamgreig promised make release this weekend.

mjepronk · 2019-07-27T17:21:52Z

That feature does not exist, right? Besides, I see that it depends on model number and on the amount of flash memory:

Low-density devices are STM32F101xx, STM32F102xx and STM32F103xx
microcontrollers where the Flash memory density ranges between 16 and 32 Kbytes.
Medium-density devices are STM32F101xx, STM32F102xx and STM32F103xx
microcontrollers where the Flash memory density ranges between 64 and 128 Kbytes.
High-density devices are STM32F101xx and STM32F103xx microcontrollers where the
Flash memory density ranges between 256 and 512 Kbytes.
XL-density devices are STM32F101xx and STM32F103xx microcontrollers where the
Flash memory density ranges between 768 Kbytes and 1 Mbyte.
Connectivity line devices are STM32F105xx and STM32F107xx microcontrollers.

The 84-byte data registers are only available in high-density, XL-density and connectivity line devices.

burrbull · 2019-07-27T17:57:28Z

One of the solution is to add feature gate for each type of microcontroller: stm32f1xxxx (stm32f103c8). They can depend on other features like mem64.
But not everyone likes this solution.

TheZoq2 · 2019-07-28T08:53:39Z

One of the solution is to add feature gate for each type of microcontroller: stm32f1xxxx (stm32f103c8). They can depend on other features like mem64.
But not everyone likes this solution.

That seems like a pretty reasonable solution to me, but I haven't heard the arguments against it

burrbull · 2019-07-28T09:05:07Z

I can make initial PR with these features.

therealprof · 2019-07-28T13:23:24Z

@TheZoq2 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.

burrbull · 2019-07-29T08:25:47Z

@mjepronk please, rebase your changes.

mjepronk · 2019-07-29T11:18:35Z

@burrbull I think my repo was already up-to-date, but I've bumped the dependency stm32f1 to version 0.8 (because your work on that repo is required for this PR).

burrbull · 2019-07-29T11:21:33Z

This repo is already on 0.8 #88 :)

mjepronk · 2019-07-29T11:24:34Z

Doh, sorry. I've merged...

therealprof · 2019-07-29T12:02:25Z

@mjepronk How about a rebase? 😉

mjepronk · 2019-07-29T13:41:34Z

@therealprof @burrbull Done, sorry for the confusion. I didn't do the push --force the first time.

TheZoq2 · 2019-07-29T17:23:19Z

This looks good to me, do we want to wait for #90 before merging?

src/backup_domain.rs

TheZoq2 · 2019-08-04T16:55:14Z

@mjepronk #90 has been merged which should be the last blocker for this, could you add the correct features for this?

mjepronk · 2019-08-05T20:17:16Z

Yes, I will pick this up ASAP. Thanks!

…32-rs#83).

mjepronk · 2019-08-09T13:56:37Z

Is there anything left to do before we can merge this?

TheZoq2 · 2019-08-09T17:13:33Z

Nope, I just missed the new pushed commits, thanks!

Also, I feel like it's time for another release, any objections to that? @therealprof

therealprof · 2019-08-09T20:15:25Z

@TheZoq2 No objections.

TheZoq2 · 2019-08-09T21:55:33Z

Cool, version 0.4.0 has been published 🎉

therealprof reviewed Jul 16, 2019

View reviewed changes