Skip to content
This repository has been archived by the owner on Jan 7, 2019. It is now read-only.

[cortex] Cleanup platform-specific code and linkerscript locations #180

Merged
merged 4 commits into from
Sep 6, 2016
Merged

[cortex] Cleanup platform-specific code and linkerscript locations #180

merged 4 commits into from
Sep 6, 2016

Conversation

salkinium
Copy link
Member

This moves the platform macros out of their respective folders and into a common file called platform.macros. The linkerscripts are moved out of the stm32 folder into the linkerscript folder

cc @ekiwi

@salkinium
Copy link
Member Author

Anyone know of a way to not have to declare the IRQ handlers in a way that they can be written both in C and C++? At the moment we have to declare them extern "C" in C++ files?

The obvious solution would be to move the interrupt table declaration into a C++ file and let the name wrangling take care of it, but then you cannot write IRQ Handlers in C anymore.

@salkinium
Copy link
Member Author

salkinium commented Sep 3, 2016

Hm, declaring them void Name_IRQHandler(void) asm("Name_IRQHandler"); seems to work, but it breaks existing extern "C" Name_IRQHandler() {} declarations.

%# In the worst case you won't even have access to the stack, if the
%# memory containing the stack is not physically enabled yet.
%# In that case, consider using inline assembly to manage stack access
%# manually, until the memory is enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Can't we execute this code after initializing the main stack?
I'd love to keep our reliance on undefined behavior to a minimum, especially in this case, where the execution order isn't obvious, as this code is pasted into the startup function.

Copy link
Member Author

@salkinium salkinium Sep 4, 2016

Choose a reason for hiding this comment

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

Writing the jump values onto the stack does not make it any less undefined.

Strictly speaking until the stack and.data and .bss sections have been initialized, we're operating in an undefined area, since the abstract C machine expects them.
Theoretically, the compiler is free to generate whatever it wants before that, incl. storing stuff on the stack for no apparent reason.

Say you moved the stack to the 4kB large battery backed SRAM in the F4, then the stack initialization would cause a bus fault, since the memory is not powered on. Therefore this code needs to be executed first.

Copy link
Member

Choose a reason for hiding this comment

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

Can we name it something more specific then?
Maybe powerUpMemory or initializeMemory?

Copy link
Member

Choose a reason for hiding this comment

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

startupCode is very generic and tells me that I can do any platform specific startup behavior here....

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it to enableInternalMemory().

@salkinium
Copy link
Member Author

I found no way to map the name-mangled IRQ handlers onto the C functions.
So we'll have to stick to adding extern "C" in front of interrupts handlers.

We could add a XPCC_INTERRUPT macro, that maps to ISR on AVR and extern "C" on ARM in C++ mode.
Maybe it could even append _vect to AVR names and _IRQHandler to ARM names, so that naming conventions are reduced to a minimum?

// On AVRs
XPCC_INTERRUPT(USART1_RX) 
-> ISR(USART1_RX_vect)

// On ARMs
XPCC_INTERRUPT(USART1) 
-> extern "C" void USART1_IRQHandler(void) // in C++ code
-> void USART1_IRQHandler(void) // in C code

Is this a terrible idea? Does it make the code look cleaner, or just like there's more magic involved?

@salkinium
Copy link
Member Author

The specific issue I have with the _IRQHandler is that the datasheet does not mention it. This is a CMSIS specific addition and with this macro we'd are able to transparently add it without the user having to know that.

@ekiwi
Copy link
Member

ekiwi commented Sep 6, 2016

I think the macros that you suggest are a good idea. They don't even break backwards compatibility!
(However, we should try to use the new macros for all signal handlers in xpcc once they are introduced.)

@salkinium salkinium merged commit e62010a into roboterclubaachen:develop Sep 6, 2016
@salkinium
Copy link
Member Author

salkinium commented Sep 7, 2016

Hm. I've played around with XPCC_INTERRUPT and it only makes things worse.
The issue is that we are extern declaring the interrupts sometimes or invoking them directly:

%% if target is stm32f3
#   define  CAN1_TX_IRQHandler      USB_HP_CAN1_TX_IRQHandler
#   define  CAN1_RX0_IRQHandler     USB_LP_CAN1_RX0_IRQHandler
#   define  CAN1_TX_IRQn            USB_HP_CAN1_TX_IRQn
#   define  CAN1_RX0_IRQn           USB_LP_CAN1_RX0_IRQn
%% endif

extern "C" void
{{ reg }}_TX_IRQHandler() {...}

extern "C" void
{{ reg }}_RX0_IRQHandler() {...}

extern "C" void
{{ reg }}_RX1_IRQHandler() {...}

%% if target is stm32f0
extern "C" void
CEC_CAN_IRQHandler()
{
    if({{ reg }}->TSR & (CAN_TSR_RQCP0 | CAN_TSR_RQCP1 | CAN_TSR_RQCP2)) {
        {{ reg }}_TX_IRQHandler();
    }

    if({{ reg }}->RF0R & (CAN_RF0R_FMP0 | CAN_RF0R_FULL0 | CAN_RF0R_FOVR0)) {
        {{ reg }}_RX0_IRQHandler();
    }

    if({{ reg }}->RF1R & (CAN_RF1R_FMP1 | CAN_RF1R_FULL1 | CAN_RF1R_FOVR1)) {
        {{ reg }}_RX1_IRQHandler();
    }
}
%% endif

I played around with a XPCC_INTERRUPT_NAME(vector) macro, but that only made things so much more obfuscated.

%% if target is stm32f3
#   define  XPCC_INTERRUPT_NAME(CAN1_TX)  XPCC_INTERRUPT_NAME(USB_HP_CAN1)
#   define  XPCC_INTERRUPT_NAME(CAN1_RX0) XPCC_INTERRUPT_NAME(USB_LP_CAN1_RX0)
#   define  CAN1_TX_IRQn            USB_HP_CAN1_TX_IRQn
#   define  CAN1_RX0_IRQn           USB_LP_CAN1_RX0_IRQn
%% endif

XPCC_INTERRUPT({{ reg }}_TX) {...}

XPCC_INTERRUPT({{ reg }}_RX0) {...}

XPCC_INTERRUPT({{ reg }}_RX1) {...}

%% if target is stm32f0
XPCC_INTERRUPT(CEC_CAN)
{
    if({{ reg }}->TSR & (CAN_TSR_RQCP0 | CAN_TSR_RQCP1 | CAN_TSR_RQCP2)) {
        XPCC_INTERRUPT_NAME({{ reg }}_TX)();
    }

    if({{ reg }}->RF0R & (CAN_RF0R_FMP0 | CAN_RF0R_FULL0 | CAN_RF0R_FOVR0)) {
        XPCC_INTERRUPT_NAME({{ reg }}_RX0)();
    }

    if({{ reg }}->RF1R & (CAN_RF1R_FMP1 | CAN_RF1R_FULL1 | CAN_RF1R_FOVR1)) {
        XPCC_INTERRUPT_NAME({{ reg }}_RX1)();
    }
}
%% endif

And extern declarations are just stupid without yet another macro XPCC_INTERRUPT_EXTERN(vector):

// otherwise implemented like this. `"C"` only in C++ env
extern "C" void XPCC_INTERRUPT_NAME(USART1)(void);

Nah, let's not.

@ekiwi
Copy link
Member

ekiwi commented Sep 7, 2016

The code which you use as an example here is actually an internal hack, that is not really intended to be used by user code. So I don't think this is necessarily a blocker.

Also there is a conceptual difference between a function definition and a function call, thus it would not be absurd to have two different macros depending on what you want to do.

For function calls I would go with something like XPCC_CALL_INTERRUPT(USART1)

@salkinium
Copy link
Member Author

Yeah, I think you're right, the macro is meant for the user, and not for the library. I'm gonna pick this up again.

@ekiwi
Copy link
Member

ekiwi commented Sep 8, 2016

For function calls I would go with something like XPCC_CALL_INTERRUPT(USART1)

Being a little bit more precise, this should probably be XPCC_CALL_INTERRUPT_HANDLER(USART1)

@salkinium salkinium deleted the feature/simplify_core_cortex branch September 15, 2016 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants