Add Schmitt Trigger Block with Uncertainty Support and Optional Interpolation #476
Conversation
wirew0rm
force-pushed
the
schmitt_trigger
branch
from
0a843d7 to
ebc8a15
Compare
There was a problem hiding this comment.
Tried to look into it but got stuck trying to get it to compile and it was not clear to me what the right direction to fix this would be.
But overall looks very nice, I suspect this was something that was changed last minute and missed a few parts of the code.
EDIT: after writing this I gave it one more quick try with more understanding and got it working. I also added fixup commits for the code formatting.
| requires(std::is_arithmetic_v<T> or (UncertainValueLike<T> && std::is_arithmetic_v<meta::fundamental_base_value_type_t<T>>)) | ||
| struct SchmittTrigger { | ||
| using value_t = meta::fundamental_base_value_type_t<T>; | ||
| using EdgeType = std::conditional_t<std::is_floating_point_v<value_t>, T, float>; // float being used for integer types |
There was a problem hiding this comment.
This is not clear to me and seems like it causes inconsistent types:
T -> EdgeType
double->doubleint ->float` <= special caseUncertainValue<double>->UncertainValue<double>UncertainValue<int>-> ``float` !!! inconsistency? would have to be handled explicitly in the rest of the code which it looks like it isn't.
| // edge timing variables | ||
| EdgeDetection lastEdge = EdgeDetection::NONE; | ||
| EdgeType lastEdgeIdx{1}; /// relative index [-EdgeType|_MAX,1[ w.r.t. current sample of the last detected edge | ||
| EdgeType lastEdgeOffset{0}; /// relative sub-sample offset [0, 1[ w.r.t. current sample of the last detected edge |
There was a problem hiding this comment.
This would mean that for integer datatype the position could only be 0 or 1?
Maybe we need an input and an output datatype?
There was a problem hiding this comment.
The thing is that -- normally it's:
float->floatdouble->doubleUncertainValue<float>->UncertainValue<float>UncertainValue<double>->UncertainValue<double>
The integer types are a bit special because while the vertical values are discrete, the time-offsets aren't, thus my choice to default to float or UncertainValue<float> for them. Was also temporarily contemplating keeping them all as float or UncertainValue<float> but this would complicate the downstream logic probably.
My goal was first to have a viable working solution and to see how it works out in the field.
wirew0rm
force-pushed
the
schmitt_trigger
branch
from
ebc8a15 to
0e521e5
Compare
RalphSteinhagen
force-pushed
the
schmitt_trigger
branch
4 times, most recently
from
0043518 to
91f9731
Compare
|
fixed tag indexing problem and upgraded unit-tests to cover some more edge cases:
|
RalphSteinhagen
force-pushed
the
schmitt_trigger
branch
2 times, most recently
from
14d6234 to
f319c57
Compare
Also: * fixed '<=>' operator for when comparing with non-UncertainValue<T> * fixed up-conversion warning in UncertainValue.hpp * upgraded UncertainValue fmt-formatter to support number format specifiers Signed-off-by: rstein <[email protected]> Signed-off-by: Ralph J. Steinhagen <[email protected]>
Signed-off-by: Ralph J. Steinhagen <[email protected]>
implements a SchmittTrigger block with edge detection and optional interpolation Signed-off-by: rstein <[email protected]> Signed-off-by: Ralph J. Steinhagen <[email protected]>
RalphSteinhagen
force-pushed
the
schmitt_trigger
branch
from
f319c57 to
4665d84
Compare
|
This PR introduces a new digitial
SchmittTriggerblock with configurable threshold and offset, as well as sub-sample edge timing interpolation options.Features
thresholdandoffsetvalues.NO_INTERPOLATION: basic detection without interpolation.BASIC_LINEAR_INTERPOLATION: simple linear interpolation between adjacent samples.LINEAR_INTERPOLATION: linear regression over the samples within the threshold window.POLYNOMIAL_INTERPOLATION: (TODO) Placeholder for future implementation using Savitzky–Golay filters.trigger_name_rising_edge,trigger_name_falling_edge)FIXME and TODOs
FIXME: In the
processBulkmethod, there's a commented-out section related to input sample buffering and history. This needs to be revisited to ensure proper operation, especially when using interpolation methods that require a history buffer. The error seems to be unrelated to the underlying algorithm and probably also chosen block implementation (tbc.). To be followed up in the GR4 if that is the case.TODO: The
POLYNOMIAL_INTERPOLATIONmethod has not yet been implemented and requires the pendingTensor<T>and SVD implementations. This will be addressed in a future update.