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

[architecture] Rework assertions to make them more useful #351

Merged
merged 4 commits into from
Mar 19, 2020

Conversation

salkinium
Copy link
Member

@salkinium salkinium commented Mar 16, 2020

This adds a reworked (breaking) model of assertions, where modm_assert() forces abandonment, and modm_assert_continue() allows for handlers to ignore assertions.
This makes it possible to distinguish between the two at the assertion site.

In addition the assertion can have a short string, that is choses completely by the user and behaves just like a normal string, not the abomination with \0 as string delimiter before (I was being a little too smart lol). And a long description which is optionally included in the binary or placed in an ELF comment section to be looked up by the short name.

Fixes #348.

TODO:

  • Update docs
  • Unsquash Commits into nicer history
  • Replace modm::ErrorReport with assertions.
  • Naming? Abandon in modm vs Abort in libc. Abandon, since abort has slightly different semantics.
  • Check if C++ exceptions for Cortex-M can be defaulted to modm_assert like for AVR: NO, because newlib nano does not define them as weak symbols, so they can't be overwritten.
  • Place descriptions into ELF comment section and integrate into build system. Better with a generic solution for logging.

@salkinium
Copy link
Member Author

salkinium commented Mar 18, 2020

This is ready for review. This is a breaking change in API, but not much change in behavior. The handlers and modm_abandon() now only get one argument passed, which is the modm::AssertionInfo struct, containing the interesting information. I've significantly updated the docs, so best read assert.md.

Tested on AVR, Cortex-M and Hosted-Darwin.

cc @rleh @se-bi

ext/gcc/newdelete_cortex.cpp Show resolved Hide resolved
%% endif
uintptr_t context;
uint8_t behavior;
} _modm_assertion_info;
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 need to call this from C code too, hence the packed struct once in C here and once in C++ as modm::AssertionInfo.


#include "assert.h"
#include <modm/architecture/interface/register.hpp>
/// @endcond
Copy link
Member Author

Choose a reason for hiding this comment

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

On purpose, there is an issue somewhere where a @code hasn't been closed, but I couldn't find it in any of the header files. (at least none that were included here).


%% if with_assert
Copy link
Member Author

Choose a reason for hiding this comment

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

Only include this if it is necessary, this is good for tiny targets.

env.substitutions = {
"with_logger": True,
"with_assert": env.has_module(":architecture:assert")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We really need to deduplicate at least all the nucleo boards. This is getting ridiculous.

src/modm/platform/can/stm32/can.cpp.in Show resolved Hide resolved
src/modm/platform/can/stm32/can.cpp.in Show resolved Hide resolved

KEEP(*(.assertion))

__assertion_table_end = . + SIZEOF(.data);
__assertion_table_end = .;
Copy link
Member Author

Choose a reason for hiding this comment

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

This bug took me a day to find. I hate AVRs so much.

@salkinium salkinium force-pushed the feature/new_assert branch 2 times, most recently from 55c1788 to 15fe379 Compare March 19, 2020 05:20
Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Nice 👌

examples/avr/assert/project.xml Outdated Show resolved Hide resolved
examples/stm32f469_discovery/assert/main.cpp Show resolved Hide resolved
ext/gcc/cxxabi.cpp.in Outdated Show resolved Hide resolved
src/modm/architecture/module.lb Outdated Show resolved Hide resolved
src/modm/architecture/module.lb Outdated Show resolved Hide resolved
src/modm/platform/can/stm32/can.cpp.in Show resolved Hide resolved
src/modm/platform/can/stm32/can.cpp.in Show resolved Hide resolved
src/modm/platform/core/cortex/delay.cpp.in Show resolved Hide resolved
Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Assert example tested on my F469 discovery board: Works 👍

@salkinium salkinium merged commit 23ec952 into modm-io:develop Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add modm_abort() as a non-recoverable modm_assert()
2 participants