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

MSP430 support #20

Closed
7 of 9 tasks
japaric opened this issue Nov 11, 2016 · 73 comments
Closed
7 of 9 tasks

MSP430 support #20

japaric opened this issue Nov 11, 2016 · 73 comments

Comments

@japaric
Copy link
Member

japaric commented Nov 11, 2016

Parent issue: #2

Next steps:

@japaric japaric changed the title MSP430 MSP430 support Nov 11, 2016
@japaric
Copy link
Member Author

japaric commented Nov 11, 2016

Experiment with a custom target out of tree

rust-lang/rust#37672 has a custom target specification in the PR description but
it has only been lightly tested. What needs to be done after that PR lands is
compiling a Rust program that actually "works", i.e. that can be flashed and
that boots correctly.

To do that, we need to know low level details about the microcontrollers:

  • How does the boot process work?
  • How is the memory address space organized? What addresses belong to RAM, which
    ones belong to Flash? etc.

And about the tooling, in particular linker scripts.

msp430-gcc ships with several linker scripts. Using the linker scripts
(memory.x and periph.x) of the msp430g2202 micro and an empty file I get
this:

$ touch empty.c

$ clang --target=msp430 empty.c

$ msp430-objdump -Cd a.out
0000f800 <__watchdog_support>:
    f800:       55 42 20 01     mov.b   &0x0120,r5
    f804:       35 d0 08 5a     bis     #23048, r5      ;#0x5a08
    f808:       82 45 00 02     mov     r5,     &0x0200

0000f80c <__init_stack>:
    f80c:       31 40 00 03     mov     #768,   r1      ;#0x0300

0000f810 <__do_copy_data>:
    f810:       3f 40 00 00     mov     #0,     r15     ;#0x0000
    f814:       0f 93           tst     r15
    f816:       08 24           jz      $+18            ;abs 0xf828
    f818:       92 42 00 02     mov     &0x0200,&0x0120
    f81c:       20 01
    f81e:       2f 83           decd    r15
    f820:       9f 4f 4a f8     mov     -1974(r15),512(r15);0xf84a(r15), 0x0200(r15)
    f824:       00 02
    f826:       f8 23           jnz     $-14            ;abs 0xf818

0000f828 <__do_clear_bss>:
    f828:       3f 40 00 00     mov     #0,     r15     ;#0x0000
    f82c:       0f 93           tst     r15
    f82e:       07 24           jz      $+16            ;abs 0xf83e
    f830:       92 42 00 02     mov     &0x0200,&0x0120
    f834:       20 01
    f836:       1f 83           dec     r15
    f838:       cf 43 00 02     mov.b   #0,     512(r15);r3 As==00, 0x0200(r15)
    f83c:       f9 23           jnz     $-12            ;abs 0xf830

0000f83e <__stop_progExec__>:
    f83e:       32 d0 f0 00     bis     #240,   r2      ;#0x00f0
    f842:       fd 3f           jmp     $-4             ;abs 0xf83e

0000f844 <__ctors_end>:
    f844:       30 40 48 f8     br      #0xf848

0000f848 <_unexpected_>:
    f848:       00 13           reti

Disassembly of section .vectors:

0000ffe0 <__ivtbl_16>:
    ffe0:       44 f8 44 f8 44 f8 44 f8 44 f8 44 f8 44 f8 44 f8     D.D.D.D.D.D.D.D.
    fff0:       44 f8 44 f8 44 f8 44 f8 44 f8 44 f8 44 f8 00 f8     D.D.D.D.D.D.D...

I expect this program should "work" if flashed into the micro. As least it
should boot properly.

Changing the C source code to:

int main() {
    while (1) {}
}

Changes the disassembly like this:

     f818:      92 42 00 02     mov     &0x0200,&0x0120
     f81c:      20 01
     f81e:      2f 83           decd    r15
-    f820:      9f 4f 4a f8     mov     -1974(r15),512(r15);0xf84a(r15), 0x0200(r15)
+    f820:      9f 4f 5a f8     mov     -1958(r15),512(r15);0xf85a(r15), 0x0200(r15)
     f824:      00 02
     f826:      f8 23           jnz     $-14            ;abs 0xf818

@@ -38,10 +38,19 @@
     f842:      fd 3f           jmp     $-4             ;abs 0xf83e

 0000f844 <__ctors_end>:
-    f844:      30 40 48 f8     br      #0xf848
+    f844:      30 40 58 f8     br      #0xf858

-0000f848 <_unexpected_>:
-    f848:      00 13           reti
+0000f848 <main>:
+    f848:      04 12           push    r4
+    f84a:      04 41           mov     r1,     r4
+    f84c:      21 83           decd    r1
+    f84e:      84 43 fe ff     mov     #0,     -2(r4)  ;r3 As==00, 0xfffe(r4)
+    f852:      00 3c           jmp     $+2             ;abs 0xf854
+    f854:      ff 3f           jmp     $+0             ;abs 0xf854
+       ...
+
+0000f858 <_unexpected_>:
+    f858:      00 13           reti

 Disassembly of section .vectors:

But compiling the following Rust program:

#![feature(lang_items)]
#![feature(no_core)]
#![no_core]
#![no_main]

#[export_name = "main"]
pub fn main() -> ! {
    loop {}
}

#[lang = "copy"]
trait Copy {}

#[lang = "sized"]
trait Sized {}

produces the same disassembly as the empty C source file. This means that the
linker is dropping all the Rust symbols. Have to figure out why that's the case.

@pftbest
Copy link

pftbest commented Nov 11, 2016

I can get a MSP430 board tomorrow, to do some tests.
Also I have a question, how does rust support __attribute__((interrupt)) declaration?
And there is no way (as far as I know) to exit low-power mode, because __bic_SR_register_on_exit built-in is not implemented in llvm.

@japaric
Copy link
Member Author

japaric commented Nov 11, 2016

how does rust support attribute((interrupt)) declaration?

It doesn't right now. We'll have to add a new ABI, e.g. extern "msp430-interrupt", to get the ABI/prelude/calling convention right. The AVR folks have done something like this, I think.

@japaric
Copy link
Member Author

japaric commented Nov 11, 2016

because __bic_SR_register_on_exit built-in is not implemented in llvm.

Could you elaborate about what this does? Is a gcc intrinsic built-in (?) that lowers to an assembly instruction? Or something else?

@pftbest
Copy link

pftbest commented Nov 11, 2016

@japaric

Could you elaborate about what this does?

It modifies the value on the stack that is located one word below the current frame (I don't know the right words how to say this). Basically when CPU enters interrupt it does this:

  1. CPU pushes status register on stack
  2. CPU jumps to interrupt handler
  3. interrupt handler pushes some stuff on the stack (it's frame)
  4. here we need to modify the value that was pushed on stack by CPU on step 1 but we can't locate it because only compiler knows how much stuff we have pushed on step 3, that's why we use built-in.
  5. interrupt handler pops it's frame from the stack and calls reti
  6. CPU pops the value from stack and writes it to status register.

As far as I know this is the only way to modify the status register and exit low power mode.

@pftbest
Copy link

pftbest commented Nov 11, 2016

@japaric
Maybe it is possible to implement this behaviour using naked functions and some assembly code, but I have not tried.

@japaric
Copy link
Member Author

japaric commented Nov 11, 2016

The built-in looks like a function call that goes inside an "interrupt handler"? Something like this:

__irq void foo() {
    // ..

     __bic_SR_register_on_exit();
}

Maybe it is possible to implement this behaviour using naked functions and some assembly code, but I have not tried.

Maybe. But I think that if you use a naked function then you have to manually modify the stack pointer during the prologue/epilogue of the function according to the number of local variables you use. My worry is that this approach may require the (final) user to deal with low level ABI-ish details.

@japaric
Copy link
Member Author

japaric commented Nov 11, 2016

The built-in looks like a function call that goes inside an "interrupt handler"?

Or is an attribute?

@pftbest
Copy link

pftbest commented Nov 11, 2016

The built-in looks like a function call that goes inside an "interrupt handler"?

Yes, it looks and feels like normal function.

Synopsis

unsigned __bic_SR_register_on_exit(unsigned mask);

Description

__bic_SR_register_on_exit clears the bits specified in mask in the saved status register of an interrupt function (i.e. it bitwise ands the complement of mask into the saved status register). This allows you to change the operating mode of the MSP430 on return from the interrupt service routine, such as changing the low power mode.
__bic_SR_register_on_exit returns the value of the saved MSP430 status register before the update.
__bic_SR_register_on_exit is an intrinsic function and produces inline code.

Note

__bic_SR_register_on_exit can only be used in interrupt functions—an error is reported if it is used outside an interrupt function.

@pftbest
Copy link

pftbest commented Nov 11, 2016

I think it is not possible to write llvm IR that will do this kind of thing. So this function should be added as intrinsic in llvm (and as built-in in clang). and only then we can have it in rust.

@japaric
Copy link
Member Author

japaric commented Nov 16, 2016

rust-lang/rust#37672 has landed and the MSP430 LLVM backend should be enabled in the next nightly. People should be able to experiment with custom targets out of tree with that nightly.

@jck
Copy link

jck commented Nov 16, 2016

I've got a couple of MSP430 fram boards. I'll try out a blinky tomorrow.

On Tue, Nov 15, 2016, 23:06 Jorge Aparicio [email protected] wrote:

rust-lang/rust#37672 rust-lang/rust#37672 has
landed and the MSP430 LLVM backend should be enabled in the next nightly.
People should be able to experiment with custom targets out of tree with
that nightly.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#20 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATR7uoRMeferu2kSHoryoaAybZB9qLOks5q-oFcgaJpZM4KvzaQ
.

@pftbest
Copy link

pftbest commented Nov 17, 2016

So I tried to compile core and I get this errors from assembler:

core.s: Assembler messages:
core.s:9468: Error: junk at end of line, first unrecognized character is `@'
core.s:9740: Error: junk at end of line, first unrecognized character is `@'
...

the code on line 9468 looks like this:

_ZN4core3num7dec2flt5rawfp10prev_float14_MSG_FILE_LINE17h5bcd06a3b0d78233E:
    .short  str.3@
    .short  27
    .short  str.1v

I tried both msp430-gcc (4.6.3) and msp430-elf-gcc (5.3.0) and both give the same errors. Then I've renamed symbol str.3@ to str.3_at in assembly and it compiled successfully. I think msp430 assembler just does not like @ in symbol names. So I see 2 options:

  1. Modify MSP430 target in LLVM so it will rename symbols with @
  2. Modify generate_local_symbol_name to not emit @ on MSP430

@tvladyslav
Copy link

Hi!
I have MSP430G2553 and I can make some tests on it. Just let me know.

@pftbest
Copy link

pftbest commented Nov 28, 2016

Anyone working on this @ symbol issue? It is blocking us from compiling core. What will be the right way to fix it? I don't know yet how hard it will be to hack it in llvm, fixing it in rustc seems easier to me.

@durka
Copy link

durka commented Dec 1, 2016

@pftbest how did you get around the LLVM assertion compiling core?

@durka
Copy link

durka commented Dec 1, 2016

@japaric re: the linker deleting everything, the problem seems to be -Wl,--gc-sections. We have to get rustc to stop passing that to the linker. I ran cargo rustc with -Z print-link-args, copied the linker command, and removed -Wl,--gc-sections and I get this disassembly:

0000c048 <start>:
    c048:	04 12       	push	r4		
    c04a:	04 41       	mov	r1,	r4	
    c04c:	21 83       	decd	r1		
    c04e:	00 3c       	jmp	$+2      	;abs 0xc050
    c050:	00 3c       	jmp	$+2      	;abs 0xc052
    c052:	ff 3f       	jmp	$+0      	;abs 0xc052

@durka
Copy link

durka commented Dec 1, 2016

OK, so cargo rustc --target=msp430 -- -C link-dead-code seems to do the trick. But I don't know why it's telling the linker that public symbols are dead in the first place.

@pftbest
Copy link

pftbest commented Dec 1, 2016

how did you get around the LLVM assertion compiling core?

So I fixed the issue in LLVM, but it is not committed in upstream yet. See #37829. Don't know how long it will take to get it into rust. Alternatively, you can just disable the assertions in configure script, the code for these functions will be bad anyway. Also there is this issue with @ in symbol names, that is not fixed yet.

We have to get rustc to stop passing that to the linker.

There should be a better way to fix this, because without gc-sections we won't be able to fit libcore in flash memory.

@japaric
Copy link
Member Author

japaric commented Dec 1, 2016

@pftbest

Anyone working on this @ symbol issue?

I didn't see this issue when compiling core but I used msp430-gcc 6.2.0.

@durka

If --gc-sections is dropping start, that means that start was never going to be called by the program. This means that the startup objects expect the user entry point to be named differently. IIRC, I tried start, _start and main but none of those worked.

nm -u crt*.o should yield some clues about the right name of the user entry point. Otherwise, we could look at the disassembly of a minimal C program; that should also give us some clues.

@durka
Copy link

durka commented Dec 1, 2016

Alternatively, we can pass -Wl,-emain to force it.

@pftbest
Copy link

pftbest commented Dec 1, 2016

@japaric

I didn't see this issue when compiling core but I used msp430-gcc 6.2.0.

I believe you saw this issue before link

It happens with both mspgcc and msp430-elf 5.4 (latest stable version from TI)

Maybe I should create a separate issue for it.

@japaric
Copy link
Member Author

japaric commented Dec 1, 2016

After re-checking with mps430-gcc 6.2, I see that core doesn't compile due to the "@" business. Weird, I tought that was already working

@pftbest
Copy link

pftbest commented Dec 1, 2016

Submitted #38116

@pftbest
Copy link

pftbest commented Dec 10, 2016

Finally managed to blink a real LED on MSP430 board. The code is here, but unfortunately, you need a patched rustc to compile it. I've put some info in README for some of the the issues I've ran into while trying to get it working.

@japaric
Copy link
Member Author

japaric commented Dec 11, 2016

Nice work, @pftbest!

re: MSP430 and MSP430X. Is -mmcu the only way to pick between those two variants during the .s -> .o step? We could add a external-asm-args field to target specifications that would let us pass arguments to msp430-elf-gcc during that step but I'd prefer if we used a more "coarse" flag to select between those variants then we'd ideally have only two built-in targets: msp430-none-elf and msp430x-none-elf and -mmcu would only be passed to the linker via RUSTFLAGS. IIRC, setting -mmcu also picks up some device-specific linker scripts so I don't think that flag should appear in the built-in target specification.

re: -nodefaultlibs. In theory we could implement the startup stuff in Rust to not depend on the startup objects that get (implicitly?) passed to the linker when -nodefaultlibs is not used thus achieving pure Rust programs like in the Cortex-M case. In any case, it makes sense to set no-default-libraries: false in the built-in target because one can manually pass -nodefaultlibs to the linker via RUSTFLAGS but the opposite (removing the -nodefaultlibs flag in the no-default-libraries: true case) is not possible without a custom target.

@pftbest
Copy link

pftbest commented May 17, 2017

Well, I think the only option that can change in the near future is no-integrated-as (and when it does, asm-args will no longer be needed).
Also there is an open question what should we do with brand new mhwmult option.

@pftbest
Copy link

pftbest commented May 17, 2017

@japaric, Can rust use llvm feature flags? Or is there any other preferred way to handle codegen options? So the problem is that some micros have hardware multiplier and some don't, and also there is 2 types of multipliers.

@awygle
Copy link
Member

awygle commented May 17, 2017

Yes, I am working on adding integrated assembler support, though I have no immediate ETA. We should still support external assemblers though - this can be overridden, right?

Let me make sure I understand the proposed targets. There are four sets of flags - rustc, llvm, assembler, and linker. We can obviously control rustc flags, and linker flags are said (above) to be able to be modified with RUSTFLAGS. Assembler flags are in the target.json, and I don't know about llvm (llc) flags.

The flags for MSP430s are cpu (msp430, msp430x, technically msp430xv2 but we can probably treat all x as xv2), hwmult (none, 16bit, 32bit, and f5series), and mcu (every micro ever). These flags are needed at the following times:

  • cpu - codegen (llvm) and assembly (just for verification that X instructions aren't used in vanilla mode)
  • hwmult - codegen (llvm) and link
  • mcu - link only

If we can pass llvm options outside the target.json but not assembly, we only need 2 targets. Otherwise, we need 8, which I think is too many.

Also - what is the expectation for targets in the compiler in terms of functionality/stability? For example, I wouldn't trust the extended instructions as far as I could throw them right now (assuming they can be generated at all, which I doubt). Does that mean we should only add the vanilla target?

@japaric
Copy link
Member Author

japaric commented May 17, 2017

Well, I think the only option that can change in the near future is no-integrated-as

The scenario in that case would be: LLVM learns to emit object files and we no longer need to use an external assembler. rustc would directly generate object files and still depend on an external linker but the asm-args flag could be dropped. The target specification would be updated to change no-integrated-as to true, drop the asm-args and to emit msp430 (not msp430x) object files.

Can rust use llvm feature flags?

There's -C llvm-args. The preferred way is to only use -C target-cpu and -C target-feature for codegen.

we only need 2 targets. Otherwise, we need 8

In my mind we only need as many targets as the number of ABIs that exist. Here by ABI I mean something like ARM's soft float vs hard float ABIs where you can't link two objects unless they have the same ABI. My main concern here is being able to link to TI's startup objects as that's required for all programs (AFAIK). IIRC, there are two ABIs in that sense: msp430 and msp430x and that's why we had to add the no-integrated-as option: the external assembler produced msp430x code by default and that couldn't be linked to the startup object which used the msp430 ABI; the no-integrated-as let us change the ABI of the Rust side to msp430.

@awygle
Copy link
Member

awygle commented May 17, 2017

In my mind we only need as many targets as the number of ABIs that exist. Here by ABI I mean something like ARM's soft float vs hard float ABIs where you can't link two objects unless they have the same ABI. My main concern here is being able to link to TI's startup objects as that's required for all programs (AFAIK). IIRC, there are two ABIs in that sense: msp430 and msp430x and that's why we had to add the no-integrated-as option

There are several issues here. First, the CPU core. TI produces two different CPU cores under the larger MSP430 "umbrella" brand. msp430 cores have 16-bit registers and can access a 64k address space. msp430x cores have 20-bit registers (mostly) and can access a 1M address space. msp430x cores also support an extended ISA which includes instructions for manipulating 20-bit registers and data values as well as some useful utility instructions like multi-push and multi-pop. libgcc builds two different versions, one for msp430x and one for msp430, the difference presumably being that the msp430x one uses the more-efficient extended instructions.

Second, the hardware multiplier. Depending on the hwmult option passed to LLVM, it will emit different library calls for integer multiplication as specified in the EABI. These functions are defined in libgcc (eventually also compiler-rt), but in separate .a files (libmul_16.a, libmul_32.a, etc). So everything uses the same ABI, calling convention, ISA, etc., but different library calls are generated and linked.

Then there's the memory models, large and small. This has to do with whether address spaces >64k are supported, and whether pointers are 16-bit or 20-bit. libgcc also builds a large-model version of itself for linking against large memory model programs. Since only msp430x CPUs support 20-bit pointers, this makes 3 versions of libgcc (and libmul_*) - small/430, small/430x, large/430x.

Also, there used to be two different MSP430 ABIs - one was in the old mspgcc, a heavily patched fork of GCC 4, while the newer one is in TI's CCS, IAR's Embedded Workbench, and upstream GCC. LLVM used to use the mspgcc ABI, but I've since patched it to use the newer ABI, since mspgcc is effectively dead. So this can be ignored now.

Finally, the startup object (crt0.o) is built as part of newlib usually, and has a large and small version but no differences based on CPU type. So just large and small.

You can link msp430 code to msp430x code as long as it's run on an msp430x CPU. You can't link large and small objects together (you might be able to but it's a nightmare). You can link to either the hwmult library you have or the software version (libmul_none.a), but you can't link to a hwmult you don't have.

Total, this gives (small/large/430)*(none/16/32/f5) is 12 configurations, eventually. The default configuration is small/none. The chip used by @pftbest in his repo is an msp430 CPU, so it needed -mcpu=msp430 in order to link to the proper libraries (msp430/none).

Personally I don't see why we can't have just one target and specify all the other stuff on the command line. Currently there's only one target triple in both LLVM and GCC.

@pftbest
Copy link

pftbest commented May 17, 2017

You can link msp430 code to msp430x code as long as it's run on an msp430x CPU.

In theory this is possible, but the linker will not allow it:

/Users/vadzim/.msp430/bin/../lib/gcc/msp430-elf/6.2.1/../../../../msp430-elf/bin/ld: error: foo.o uses MSP430 instructions but /Users/vadzim/.msp430/bin/../lib/gcc/msp430-elf/6.2.1/../../../../msp430-elf/lib/crt0.o uses MSP430X
/Users/vadzim/.msp430/bin/../lib/gcc/msp430-elf/6.2.1/../../../../msp430-elf/bin/ld: failed to merge target specific data of file foo.o
collect2: error: ld returned 1 exit status

So they should be a separate targets.

Multiplier option on the other hand is not so strict, for example, gcc will happily link two object files, where one was compiled with -mhwmult=32bit and the other with -mhwmult=none. So for this option we probably don't need to create separate targets.

What can be done about small/large memory models I'm not sure yet.

@awygle
Copy link
Member

awygle commented May 17, 2017

In theory this is possible but the linker will not allow it

You're right, I don't know why I didn't notice the 430/ folder for crt0. Curious that they chose to do this but I suppose it would almost always be an error.

@pftbest
Copy link

pftbest commented May 17, 2017

I think we would need only 3 targets in the end:

  • msp430
  • msp430x-small
  • msp430x-large

But right now we only have the first one working, so if we are going to merge something, I think we should only do the msp430.

@japaric
Copy link
Member Author

japaric commented May 18, 2017

Multiplier option on the other hand is not so strict, for example, gcc will happily link two object files, where one was compiled with -mhwmult=32bit and the other with -mhwmult=none

Possibly because all the operations that would involve the hardware mulitplier get lowered to calls to intrinsics by gcc and are left undefined in object files. Thus the only -mhwmult that matters is the one used to link the final binary; that one selects the appropriate libmul_foo.a library.

If that hypothesis is correct then today, where the intrinsics are written in C and packaged in libmul_foo.a, the way to select the hwmult setting is via the -C link-arg=hwmult=foo rustc flag.

In a 100% Rust scenario hwmult selection could be emulated by having Cargo features in the compiler-builtins crate (that crate only appears once and at the bottom of the dependency graph); in that scenario, targets won't have a hwmult setting and the user would pick the hwmult through a Cargo feature (e.g. in Xargo.toml).

So yeah it sounds to me that at the end we'll want the three targets that @pftbest mentioned.

I think we should only do the msp430.

Sounds reasonable to me.

@awygle
Copy link
Member

awygle commented May 18, 2017

I agree all of with the above.

@pftbest
Copy link

pftbest commented Jun 21, 2017

We have some new ABI related bugs.
First the #[repr(C)] struct Foo { a: i8 } has 2 byte size in rust but 1 byte size in clang. This might be a bug in the data layout string.

Also this C code doesn't follow EABI

void gg(char a, char b, char c, char d, char e, char f);
void kk() {
    gg(1, 2, 3, 4, 5, 6);
}

gcc:

	SUB.W	#2, R1
	MOV.B	#6, 1(R1)
	MOV.B	#5, @R1
	MOV.B	#4, R15
	MOV.B	#3, R14
	MOV.B	#2, R13
	MOV.B	#1, R12
	CALL	#gg

llvm:

	sub.w	#4, r1
	mov.w	#6, 2(r1)
	mov.w	#5, 0(r1)
	mov.w	#1, r12
	mov.w	#2, r13
	mov.w	#3, r14
	mov.w	#4, r15
	call	#gg

@awygle
Copy link
Member

awygle commented Jun 21, 2017

The latter issue seems to be due to the promotion of i8s to i16s in AnalyzeVarArgs in MSP430ISelLowering.cpp. We might be able to just delete that whole block and call it done.

@pftbest
Copy link

pftbest commented Jun 21, 2017

I found it because of this comment in callingconv.td

  // Integer values get stored in stack slots that are 2 bytes in
  // size and 2-byte aligned.
  CCIfType<[i16], CCAssignToStack<2, 2>>

The first issue is indeed a bug in our data layout string. It shouldn't have a:16 spec in it.
When I remove it, everything works as expected.
This also means that clang is ignoring layout string for some reason (or it also has a bug)

@pftbest
Copy link

pftbest commented Jun 21, 2017

We might be able to just delete that whole block and call it done.

There is an exception for varargs in EABI, they should be promoted to i16 still.

@pftbest
Copy link

pftbest commented Jul 10, 2017

Hello, @awygle.
To be consistent with cortex-m-rt, we need to enable interrupts before entering main() in msp430-rt.
Is it safe to do on msp430? Can some peripheral interrupt stay enabled after MCU reset?

@japaric
Copy link
Member Author

japaric commented Jul 11, 2017

Note that cortex-m-rt does nothing special about interrupts before main. However, Cortex-M devices behave like this: on reset all the interrupts are unmasked (PRIMASK = 0), but each interrupt must be enabled (using the NVIC) before it can be used. With code:

fn main() {
    // ..
    nvic.set_pending(Interrupt::TIM2); // no effect

    nvic.enable(Interrupt::TIM3);
    nvic.set_pending(Interrupt::TIM3); // ISR preempts main

    nvic.enable(Interrupt::TIM4);
    interrupt::disable(); // PRIMASK = 1 (_masks_ all interrupts)
    nvic.set_pending(Interrupt::TIM4); // no effect
}

IMO, msp430-rt shouldn't do anything special to try to match the behavior of Cortex-M devices; msp430-rt should preserve the default behaviar of the MSP430 architecture.

@awygle
Copy link
Member

awygle commented Jul 11, 2017

Hi @pftbest,

All peripherals should be reset after MCU reset. However, be aware that LPM3.5 allows the RTC module to stay active but triggers what looks like a reset on wakeup. See pages 60 and 61 of http://www.ti.com/lit/ug/slau367n/slau367n.pdf for more details. Basically, it will reset itself but that might not be what you actually want.

@pftbest
Copy link

pftbest commented Jul 11, 2017

msp430-rt should preserve the default behavior of the MSP430 architecture.

Well, then we would always have a one more unsafe block inside the main().

And in general I think that current system doesn't work too well with MSP430.
For example, lets look at the timer example, it boils down to:

fn main() {
    // interrupts are disabled here, `free` is useless
    interrupt::free(|cs| {
        let timer = TIMER0_A3.borrow(cs);
        // ...
    });

    // mandatory unsafe block
    unsafe {
        interrupt::enable();
    }

    loop {}
}

interrupt!(TIMER0_A1, timer_handler);
fn timer_handler() {
    // interrupts are disabled here, so this `free` is also useless
    interrupt::free(|cs| {
        let timer = TIMER0_A3.borrow(cs);
        // ...
    });
}

I think a better system for MSP430 may look like this:

fn main(cs: CSToken) { // cs token by value

    let timer = TIMER0_A3.borrow(&cs); // borrow it to modify registers
    // ...

    // when we are done, we can give up the token to enable interrupts
    interrupt::enable(cs); // no unsafe block required

    // here we would need the `free` function, since we don't have a token anymore
    interrupt::free(|cs| {
        // ...
    });

    loop {}
}

interrupt!(TIMER0_A1, timer_handler);
fn timer_handler(cs: CSToken, sr: SavedSr) { // cs token by value, sr for LPM support
    let timer = TIMER0_A3.borrow(&cs);
    // ...

    // if we want to enable interrupts inside an interrupt handler,
    // same story
    interrupt::enable(cs); // no unsafe block required

    // this thing will do __bic_sr_on_exit
    sr.exit_lpm();
}

@japaric
Copy link
Member Author

japaric commented Jul 11, 2017

And in general I think that current system doesn't work too well with MSP430.

Sure. I don't really like or directly use the cortex-m-quickstart system either. Quickstart is meant to give you one way, ideally the simplest way (vanilla main and no weird (proc) macros), to write data race free code but it's not necessarily the most ergonomic or the most efficient way to write data race free code.

cortex-m-rt and svd2rust should serve as a good substrate to build better systems to achieve data race freedom though. The better system you mention could be achieved with macros like I have mentioned you before. Something like this:

app! {
    device: msp430g2553,
    init: init,
    idle: idle,
}

// full access to all peripherals
fn init(p: msp430g2553::Peripherals) {
    p.PORT1.write(..);
}

fn idle() -> ! {
    loop {
        interrupt::free(|cs| {
            // do something
        });

        // do something else
    }
}

where app! expands into something like this:

fn main() {
    init(unsafe { msp430g2553::Peripherals::all() });
    unsafe { interrupt::enable() }
    idle()
}

Of course you could also modify the msp430 crate to approach the system you've described. You are the maintainer of those crates; you make the decisions here.

@japaric japaric added this to the Epoch 2018 milestone Feb 22, 2018
alexeicolin added a commit to CMUAbstract/libmspbuiltins that referenced this issue May 26, 2018
This definition is wrong, because the offset on the stack is not
a constant. See this link for some ideas for how to actually do it:
rust-embedded/wg#20 (comment)
@japaric
Copy link
Member Author

japaric commented Jul 3, 2018

Triage (last WG meeting).

The goal is to have a rust-std component working on stable plus up to date svd2rust support by the
edition release. The final deadlines for those are:

  • 1.29 (2018-09-13). rust-std component landed and enabled in rust-lang/rust

  • 1.30 (2018-10-25). svd2rust support up to date. This requires updating the msp430-rt crate.

Compiler support for interrupts will be left unstable for now. The MSP430 community can explore
interrupt support using FFI (at a cost), but having a full solution for that is not a goal for the
edition release.

@japaric japaric modified the milestones: 2018 edition, RC1 Jul 27, 2018
@cr1901
Copy link

cr1901 commented Aug 6, 2018

I would add:

  • 1.29 (2018-09-13). msp430 codegen working again (currently blocked on "cannot select" llvm error for atomics).
  • 1.29 (2018-09-13) msp430 has first-class Cargo support? (i.e. no more need to compile your own Xargo.)

Compiler support for interrupts will be left unstable for now... having a full solution... is not a goal for the edition release.

Does this mean up-to-date RTFM is a non-goal as well?

@japaric
Copy link
Member Author

japaric commented Aug 6, 2018

@pftbest this thread has gotten too long. Could you make new issues for the stuff you want to get done by the edition release and assign them tentative milestones (RC1, RC2; see this repo milestones)? Upstream stuff like rust-std components and LLVM bugs should be filled here; everything else should be filled in their own repo.

@cr1901 RTFM is not a blocker / priority for the edition release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants