Stir register and debugger check

[thenewwazoo]

Adds support for requesting an interrupt via the STIR register and checking whether a debugger is attached.

[thenewwazoo]

(This is a re-submittal of #91, which I couldn't figure out how to re-open, sorry)

[korken89]

This LGTM, but I will see if anyone else of the team has a comment. Strangely though, Travis has not run. Do you know why @japaric ?

[korken89]

bors try

[bors]

try

Build failed

• continuous-integration/travis-ci/push

[korken89]

There were some errors in the build, for nightly it seems to be in bare-metal can you open an issue for this?

[thenewwazoo]

I'm sitting on this right now because, for reasons I can't figure out, the is_debugger_attached() method doesn't appear to work as intended and I want to figure out why. In short,

(gdb) i r r0
r0             0xe000edf0	-536810000
(gdb) stepi
core::ptr::read_volatile::h4b0ffdf9b2e18c09 (src=0xe000edf0) at /checkout/src/libcore/ptr.rs:472
472	/checkout/src/libcore/ptr.rs: No such file or directory.
(gdb) disas
Dump of assembler code for function core::ptr::read_volatile::h4b0ffdf9b2e18c09:
=> 0x08003d0a <+0>:	ldr	r0, [r0, #0]
   0x08003d0c <+2>:	bx	lr
End of assembler dump.
(gdb) stepi
halted: PC: 0x08003d0c
473	in /checkout/src/libcore/ptr.rs
(gdb) i r r0
r0             0x0	0

I'm not sure if there's some precondition I'm missing to successfully reading DHCSR.

[japaric]

Could you include in the documentation why would one prefer this over set_pending and also mention that this API is not available on ARMv6-M.

[thenewwazoo]

I'll be honest... I don't know why to choose one vs the other. The reference manuals don't make the different clear. :) I'll add a note about not being available on armv6m

[japaric]

@thenewwazoo did you get the is_debugger_attached method to work? i.e. is this ready for review? Actually, I'm going to let @korken89 finish the review. :-)

[korken89]

I will give the PR a spin tonight :) If I find any issue I will report back!

[thenewwazoo]

I was not able to get the is_debugger_attached method to work, but @adamgreig reported the same approach has historically worked for him on other devices than I've got on-hand. It's one of those things that should work... In any case, I don't believe I'm missing any preconditions.

[korken89]

I have it a quick test, and it's not working for me. I will dig deeper tomorrow and see what's missing.

[adamgreig]

I've just tested this on a STM32F407 and it works as expected. In my case I flash an LED while debugger is attached and don't if not. I'll see if I can find a few other targets to test it on this week.

However I suggest replacing it with:

impl DCB {
    /// Is there a debugger attached?
    pub fn is_debugger_attached() -> bool {
        unsafe { (*Self::ptr()).dhcsr.read() & 0x1 == 1 }
    }
}

to make it accessible without an instance of DCB, just like DWT::get_cycle_count() and friends.

[japaric]

However I suggest replacing it with:

That'd be fine iff reading DHCSR does not clear any of its bits. (NB I haven't read this register documentation so I'm just asking for caution rather than saying this should not be done)

[japaric]

If this is not working really perhaps if one of those optional things that vendors may or may not implement? If that's the case it should be noted in the docs.

[adamgreig]

Checking the ARM, for DHCSR,

Access to the DHCSR from software running on the processor is IMPLEMENTATION
DEFINED

And while this technique is suggested by ARM (i.e., there's no other 'correct' way to do it AFAIK), it also appears that on Cortex-M0 the CPU can't read this register.

That'd be fine iff reading DHCSR does not clear any of its bits.

Annoyingly S_RESET_ST and S_RETIRE_ST are cleared by reading DHCSR, though I'm not sure if the CPU reading it will trigger the clear or only a debugger reading it. In any event S_RETIRE_ST is set to 1 whenever the processor completes an instruction, so just reading DHCSR would set the bit anyway. I wonder if it might be justifiable for this scenario. It's not like the exclusive access ensured by having a DCB object in any way synchronises access with the debugger anyway.

[thenewwazoo]

Great find, @adamgreig!

It looks like I can gate this on armv6m, and build a more robust test for cortex-m0 targets in a separate crate. I also agree that making the is_debugger_attached method into a function would be better.

[adamgreig]

Even on armv6m I'm not sure it's actually never going to work, just it seems like most Cortex-M0 don't permit it. I can't find anything in the ARMv6-M ARM to say it's prohibited, just the same caution that it's implementation defined as on the ARMv7-M ARM.

build a more robust test for cortex-m0 targets

Can this be done? I can't see any good way to check for this on Cortex-M0.

[korken89]

I have now tested this on a Cortex-M4 and Cortex-M3 where it was working, however on a Cortex-M0 (as discussed above) it is not working.

On S_RESET_ST, as I see it this is only for debugger use as anyone implementing something depending on happening after reset would be done through pre_init.
Are there any more comments or change requests?

@rust-embedded/cortex-m

[adamgreig]

I'm happy with this as-is, but wouldn't mind a second note in the comments that it reads DHCSR and therefore clears S_RESET_ST and S_RETIRE_ST, just in case some poor soul from the future gets stuck hunting down something annoying.

The existing note could maybe also be clarified to just say "This function's operation is implementation defined and is not functional on Cortex-M0 devices".

[korken89]

@thenewwazoo Can you add these comments and I will merge this?

[korken89]

Thanks for the quick addition @thenewwazoo !
There seems to be a problem with one of the tests, could you give it a quick look?

[thenewwazoo]

I looked into the test, and I've got a proximate cause.

112    assert_eq!(address(&nvic.stir), 0xE000EF00);

is the line that fails. address(&nvic.stir) evaluates to 0xe000e930, but according to the TRM, 0xE000EF00 is the correct value.

[thenewwazoo]

The test was executing for armv6m targets too (where the register doesn't exist), so I added a cfg attribute to skip it. The current failure is when building for x86 targets, where it is similarly nonsensical.

I figure I can:

• also deny the test for x86 targets
• refactor the tests to only permit armv7m, armv7em, ...?
• adjust the reserved5 size values in nvic.rs so all platforms line up and the test "succeeds"
• leave it as-is

[korken89]

Thanks for checking and finding the cause @thenewwazoo ! Checking the tests and the CI scripts I am not sure why the x86 target is run at all. @rust-embedded/cortex-m am I missing something obvious? Else I will opt for removing the cargo test and only keep the cargo check for the x86 target.

[japaric]

I'm still not happy about this being a static method that modifies the contents of the register. The documentation says:

Writes to this register in any size other than word are Unpredictable. It is acceptable to read in any size, and you can use it to avoid or intentionally change a sticky bit.

Could you turn this into a byte read to avoid clearing the S_RESET_ST and S_RETIRE_ST bits? Then it'd be fine to keep this as a static method.

[thenewwazoo]

I've tried a couple of ways to do this, but am not sure which you'd prefer.

Byte-wise access:

pub struct RegisterBlock {
    /// Debug Halting Control and Status
    pub dhcsr: [RW<u8>;4],
...

breaks writing to the register, which requires that the high halfword always be 0xA05F.

Using an untagged union:

pub union DHCSR {
    /// 32-bit word access
    w: WO<u32>,
    /// Individual byte access
    b: [RW<u8>; 4],
}

/// Register block
#[repr(C)]
pub struct RegisterBlock {
    /// Debug Halting Control and Status
    pub dhcsr: DHCSR,
...

looks like a good solution, but untagged unions of non-Copy types is unstable.

I could try to do some unsafe trickery by doing pointer math but that seems like a bad idea. What do you think?

As an aside, I'm sorry this tiny little change has eaten up so much energy. It seemed so simple at first. :(

[thenewwazoo]

I was reminded of this today - any suggestions? I'm happy to go with whatever's best, I just don't know what it is. :)

[japaric]

@thenewwazoo sorry for dropping the ball here

breaks writing to the register, which requires that the high halfword always be 0xA05F.

this would be a breaking change; also it breaks writes as you say

Using an untagged union:

this is unstable so it's a no-go

I could try to do some unsafe trickery by doing pointer math but that seems like a bad idea.

using pointers seems like the only stable, backward-compatible solution. You can write something like (*(&dhcsr as *const [RW<u8>; 4]))[index].read()) to avoid the pointer math. Though pointer math is probably easier to read ptr::read_volatile((&dhcsr as *const u8).add(index)).

[thenewwazoo]

@Disasm @adamgreig this should be GTG (perhaps pending @japaric confirming his requested changes have been satisfied)

[adamgreig]

I believe all outstanding objections have been addressed, thanks @thenewwazoo!

bors r+

Requested changes have been addressed

[bors]

Build failed (retrying...)

• continuous-integration/travis-ci/push

[bors]

Build failed (retrying...)

• continuous-integration/travis-ci/push

[bors]

Canceled

[thenewwazoo]

Tests should succeed now, having fixed the reserved offsets

[adamgreig]

thanks!

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push