[wip] xpcc error model using assertions #185
Conversation
|
|
||
| using namespace xpcc::at90; | ||
|
|
||
| namespace Board |
There was a problem hiding this comment.
Could you please add more detailed information about the board, e.g. including a link to the documentation?
| using xpcc::accessor::asFlash; | ||
|
|
||
| // Flash support on avr-gcc is so horribly broken. | ||
| #define IFS(s) asFlash(PSTR(s)) |
There was a problem hiding this comment.
The AVR is pretty cool, but this Flash f-up is pretty unforgiveable. This should be solved at a compiler level, not a user application level though.
| { | ||
| serialStream << IFS("#1: '") << asFlash(module) << IFS("'!") << xpcc::endl; | ||
| // The strings are located in FLASH!!! | ||
| if (strcmp_P(module, PSTR(XPCC_IOBUFFER_MODULE_NAME)) == 0) |
There was a problem hiding this comment.
This is a bit sucky, if your application should be portable between AVR and Cortex-M. But I see not other way than to place the strings into Flash, since this feature is supposed to be used on ATtinys too.
|
|
||
| #define XPCC_CAN_MODULE_NAME "can" | ||
| #define XPCC_IOBUFFER_MODULE_NAME "iobuffer" | ||
| #define XPCC_UART_MODULE_NAME "uart" |
There was a problem hiding this comment.
These are supposed to be declared by the module, perhaps overwriteable?
| @@ -75,6 +75,7 @@ | |||
| #define EXTERN_FLASH_STORAGE_STRING(s) extern const char s[] | |||
|
|
|||
| #define INLINE_FLASH_STORAGE_STRING(s) ((const char *)(s)) | |||
There was a problem hiding this comment.
I don't know what I was smoking when I thought that INLINE_FLASH_STORAGE_STRING("Hello World!") was an acceptable length for a macro to be used inline.
| @@ -31,7 +31,7 @@ | |||
|
|
|||
| # ----------------------------------------------------------------------------- | |||
| def avrdude_flash(env, source, eeprom_source='', alias='avrdude_program'): | |||
| actionString = '$AVRDUDE -p $AVRDUDE_DEVICE -c $AVRDUDE_PROGRAMMER -P $AVRDUDE_PORT $AVRDUDE_OPTIONS -U flash:w:' | |||
| actionString = '$AVRDUDE -V -p $AVRDUDE_DEVICE -c $AVRDUDE_PROGRAMMER -P $AVRDUDE_PORT $AVRDUDE_OPTIONS -U flash:w:' | |||
There was a problem hiding this comment.
Maybe we should add a avrdude.verify={true, false} to project.cfg?
| #endif | ||
|
|
||
| #define xpcc_assert(condition, module, location, failure) \ | ||
| xpcc_assert_evaluate((condition), INLINE_FLASH_STORAGE_STRING(module "\0" location "\0" failure)); |
There was a problem hiding this comment.
The reason why I chose to put this all into one string is that only one address needs to be loaded into registers. This should make assertions a lot less costly on AVRs, since only 3 registers (2 on ARM) need to be written (one for bool, two for address).
| .xpcc_assertion : AT( ADDR(.text) + SIZEOF(.text) + SIZEOF(.data)) | ||
| { | ||
| . = ALIGN(2); | ||
| __assertion_table_start = . + SIZEOF(.data); |
There was a problem hiding this comment.
This is an issue of VMA vs. LMA. This is placed physically after data into Flash, but virtually after flash. ld is not really well documented, so maybe I just don't know any better. But the end result is the same anyways.
| KEEP(*(.assertion)) | ||
| __assertion_table_end = .; | ||
| . = ALIGN (4); | ||
|
|
There was a problem hiding this comment.
Snippets like these should be deduplicated.
|
Any further review comments by the usual suspects (cc @ekiwi @dergraaf @strongly-typed)? |
Well I am opposed to the use of strings for this purpose. However, if no one else objects, I think you should merge. |
What is your alternative? (this is mine). |
I don't have one which is why I won't object to merging this even though I do not necessarily like this solution. |
I'm working on adding a universal error model to xpcc via assertions that can be modified by the user application.
The principle is described here in detail and was inspired by trying to find a solution to #114.
Note that this is very much WIP, and none of the interfaces are finalized. I plan to write a blog post about this for additional concept documentation.
I made the decision to have three fields, so that people are not tempted just to put a human readable string in there, which would defeat the purpose of the handlers.
Also see this alternate implementation with binary errors. It's not very useful.
cc @ekiwi @dergraaf @strongly-typed