Conversation
1b2e853 to
7db5e3f
Compare
Both boards still called the pre-merge ZP_PARAM::get(id) single-argument form when deciding whether to construct the rangefinder. hwbuild.bash does not compile boardfiles (it builds only libzeropilot4.0.a), so the library build could not catch this; it surfaced from a direct arm-none-eabi-g++ syntax pass over every boardfile in both vehicle configurations.
| constexpr ZP_Error ZP_ERROR_MEMORY_OVERFLOW {1u << 13}; // Buffer or FIFO queue overflow | ||
| constexpr ZP_Error ZP_ERROR_TIMEOUT {1u << 14}; // Operation timed out | ||
| constexpr ZP_Error ZP_ERROR_NACK {1u << 15}; // Peripheral did not acknowledge | ||
| constexpr ZP_Error ZP_ERROR_EXT_API {1u << 16}; // OR'd with a code above: failure came from HAL/RTOS/FatFs |
There was a problem hiding this comment.
Would suggest ZP_ERROR_INVALID_ID or something along those lines so that instead of returning ZP_ERROR_FAIL for invalid ID access, we return a more specific error code.
There was a problem hiding this comment.
Would there be a scenario where ZP_ERROR_INVALID_ARG would not suffice
There was a problem hiding this comment.
yeah if an ID doesnt exist, invalid ARG might indicate that there was something wrong with the argument used rather than an ID, say a CAN node ID, doesn't exist. it's clearer what's happening, no reason to not have it
There was a problem hiding this comment.
I don't see a scenario where we ever encounter an invalid CAN ID. I also don't think there is an existing module that could return this as an error code. I think it would be better to introduce this error when it is actually used rather than having a whole bunch of error codes that never get used
This reverts commit 18d3925.
| @@ -0,0 +1,170 @@ | |||
| #include "zp_bit.hpp" | |||
There was a problem hiding this comment.
rename bit folder to zp_bit to match param and error folders?
| constexpr ZP_Error ZP_ERROR_MEMORY_OVERFLOW {1u << 13}; // Buffer or FIFO queue overflow | ||
| constexpr ZP_Error ZP_ERROR_TIMEOUT {1u << 14}; // Operation timed out | ||
| constexpr ZP_Error ZP_ERROR_NACK {1u << 15}; // Peripheral did not acknowledge | ||
| constexpr ZP_Error ZP_ERROR_EXT_API {1u << 16}; // OR'd with a code above: failure came from HAL/RTOS/FatFs |
There was a problem hiding this comment.
yeah if an ID doesnt exist, invalid ARG might indicate that there was something wrong with the argument used rather than an ID, say a CAN node ID, doesn't exist. it's clearer what's happening, no reason to not have it
1f77790 to
97d32d9
Compare
938616b to
ba6f217
Compare
Penguronik
left a comment
Penguronik
left a comment
There was a problem hiding this comment.
Just had a few quick qs nonblocking, nice stuff
| } | ||
|
|
||
| // Reception is aborted by the error, and nothing else would ever start it again | ||
| (void)telemLinkHandle->restartRx(); |
There was a problem hiding this comment.
Is this too silent? Any chance we can log it somewhere or just have a clear TODO for logging it somewhere after logging pr gets merged so we dont miss it
| cmake_minimum_required(VERSION 3.12) | ||
| project(zeropilot4.0) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 17) |
There was a problem hiding this comment.
Any reason we're upping the standard btw, its ok just curious
| #include "unit_conversions.hpp" | ||
| #include <limits> | ||
|
|
||
| static inline float readParam(ZP_Error &result, ZP_PARAM_ID id) { |
There was a problem hiding this comment.
Is this AM specific or something youd want to be more general?
| ScaledImuBatch_t scaledImuData = {}; | ||
| ZP_Error imuStatus = imuDriver->readRawData(imuData); | ||
| imuStatus |= imuDriver->scaleIMUData(imuData, scaledImuData); | ||
| (void)imuStatus; |
There was a problem hiding this comment.
Whats the point of this line imuStatus gets used later it seems
| setArmFlag = false; | ||
|
|
||
| systemUtilsDriver->profilerEnd(profilerId); | ||
| return; |
There was a problem hiding this comment.
Not really this prs concern i guess but single point of return would be nice since we have stuff like profiler that we need to remember calling before a return statement
| if (REPORT_TICK) { | ||
| const MAV_SEVERITY SEVERITY = (BIT_CONFIG[i].level == BitLevel_e::CRITICAL) ? MAV_SEVERITY_CRITICAL : MAV_SEVERITY_WARNING; | ||
| result |= sendStatusTextToTelemetryManager(SEVERITY, BIT_HANDLERS[i].failText); | ||
| } |
There was a problem hiding this comment.
Ah ok so no more function ptrs and any action would be taken for like disarm in here then? As well as maybe any other manager would check this bit and act themselves as well?
7 participants
This PR adds error handling as defined in
ZP_Error. It also adds an independent PBIT and CBIT module to report different severities of different errors. This reduces the amount of logic in SM, AM, and TM needed for error handling.This PR also raises the standard from C++14 to C++17.