Ease of use improvements#38
Merged
Merged
Conversation
Also made that method work with parameter names instead of only parameter identifiers
The check should be performed on all traces that are added, and it should always be done
…modification
This allows for constructions like `new Trace(SampleCoding.FLOAT, [0] * sample_count, TraceParameterMap().add('input', bytes.fromhex('0102030405060708')))`
Issues were: incorrect comparison between ndarrays and lists, no support for multi-dimensional arrays, and no detection of empty ndarrays.
1157e50 to
6da200f
Compare
More explicitly set types of ndarray input values
6da200f to
76da810
Compare
matthijsgeers-riscure
suggested changes
Nov 16, 2022
|
|
||
| def insert(self, name: str, type: ParameterType, size: int, offset: int): | ||
| """Insert a trace parameter definition into this map in a specified location. If the given offset would put the | ||
| new TraceParameter in the middle of a parameter already present in the map, the offset is increased to put add |
There was a problem hiding this comment.
Either put or add is redundant here
| def insert_std(self, name: str, size: int, offset: int): | ||
| """Insert a trace parameter definition of a StandardTraceParameter into this map in a specified location. If | ||
| the given offset would put the new TraceParameter in the middle of a parameter already present in the map, the | ||
| offset is increased to put add the new parameter after that existing one instead. Any parameters already present |
There was a problem hiding this comment.
Either put or add is redundant here
| type = StandardTraceParameters.from_identifier(name).parameter_type | ||
| self.insert(name, type, size, offset) | ||
| except ValueError: | ||
| raise ValueError(f"No StandardTraceParameter found with name '{name}'. Either specify a type in this " |
There was a problem hiding this comment.
Specifying a type is not possible in this method, right? Maybe you could reference the non-standard one instead?
| type = StandardTraceParameters.from_identifier(name).parameter_type | ||
| self.append(name, type, size) | ||
| except ValueError: | ||
| raise ValueError(f"No StandardTraceParameter found with name '{name}'. Either specify a type in this " |
There was a problem hiding this comment.
Same comment applies here; shouldn't you reference append in this error message instead?
|
|
||
| :param name: The name of the TraceParameter for which to add a definition. This name must match that of a | ||
| StandardTraceParameter | ||
| :param size: The size of the TraceParameter, in number of values of its type |
There was a problem hiding this comment.
I think number of values of its type is a confusing way to describe this parameter. It suggests to me that the type can have a number of values. How about simply number of elements instead?
Make sure to update this for the other occurrences, too
| """Test whether this RawTraceData could be interpreted by given definitions | ||
|
|
||
| :param definitions: The trace parameter definition map of the trs file to which the trace with this trace | ||
| raw trace data will be added |
There was a problem hiding this comment.
This sentence is not what you think it is :P
| return isinstance(other, type(self)) and self.value == other.value | ||
| if not isinstance(other, type(self)): | ||
| return False | ||
| if (type(self.value) == list or type(self.value) == ndarray) and \ |
There was a problem hiding this comment.
These lines are a bit confusing. Can you think of a way to make them more readable?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ease-of-use improvements based on user feedback.
PR also contains several fixes for bug encountered while implementing these improvements.