v5.2.0 #220
Open
v5.2.0 #220
Conversation
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
Better separation of concerns between computations and plotting logic, and switch to composition instead of inheritance for the graph creators. Also added better type safety using Protocols, better abstract base classes and data transfer objects.
this will allow to manually specify a specific chip for the measurement and override the default behavior where S&T usually try to find the best suited chip based on what is found in `[resonance_tester]` config section
Refactor graph creators And fix CLI mode
in order to help mitigate error and failures on printer having low pewer host computer with CANbus, etc...
Reviewer's GuideThis PR overhauls the graph creation subsystem into a composition-based architecture—decoupling computation and plotting logic into dedicated modules—while enhancing CLI support, adding an ACCEL_CHIP override to relevant macros, and improving belt measurement robustness with an extended dwell time. Sequence Diagram: Graph Creation Process with New ArchitecturesequenceDiagram
actor ClientCode
participant GCF as GraphCreatorFactory
participant SGC as SpecificGraphCreator
participant SC as SpecificComputation
participant SP as SpecificPlotter # Instance
participant MM as MeasurementsManager
ClientCode ->> GCF: create_graph_creator(type, config)
GCF -->> SGC: new SpecificGraphCreator(config, SpecificComputationClass, SpecificPlotterClass)
note right of SGC: SGC holds SpecificComputationClass and an instance of SpecificPlotter (SP)
GCF -->> ClientCode: specificGraphCreator
ClientCode ->> SGC: configure(...)
ClientCode ->> SGC: create_graph(measurementsManager)
activate SGC
SGC ->> SGC: computation_instance = _create_computation(measurementsManager)
activate SGC
SGC ->> SC: new SpecificComputation(measurementsManager, config_from_SGC)
SC -->> SGC: computation_instance
deactivate SGC
SGC ->> SC: computationResult = compute()
activate SC
SC -->> SGC: computationResult
deactivate SC
SGC ->> SP: figure = plot(computationResult)
activate SP
SP -->> SGC: figure
deactivate SP
SGC ->> SGC: _save_figure(figure)
deactivate SGC
Sequence Diagram: ACCEL_CHIP Parameter Handling in MacrossequenceDiagram
actor User
participant Macro as axes_shaper_calibration
participant Printer
participant AccelHelper as Accelerometer
User ->> Macro: AXES_SHAPER_CALIBRATION(..., ACCEL_CHIP=chip_name_or_None)
Macro ->> Macro: current_accel_chip = Get ACCEL_CHIP param
alt current_accel_chip is None (not provided by user)
Macro ->> AccelHelper: find_axis_accelerometer(printer, axis)
AccelHelper -->> Macro: found_chip_name
Macro ->> Macro: current_accel_chip = found_chip_name
end
alt current_accel_chip is None (still not found)
Macro -->> User: Error: No suitable accelerometer found
else chip found or provided
Macro ->> Printer: k_accelerometer = lookup_object(current_accel_chip)
alt k_accelerometer is None (chip not valid)
Macro -->> User: Error: Accelerometer chip not found
else chip valid
Macro ->> AccelHelper: new Accelerometer(k_accelerometer, reactor)
AccelHelper -->> Macro: accelerometer_instance
Macro ->> Macro: Proceed with measurements using accelerometer_instance
end
end
Class Diagram: GraphCreator Factory and RegistrationclassDiagram
class GraphCreator {
<<Abstract>>
+registry: dict
+graph_type: str
+register(graph_type: str) classmethod
+__init__(config, computation_class, plotter_class)
}
class GraphCreatorFactory {
+create_graph_creator(graph_type: str, config: ShakeTuneConfig) GraphCreator
}
GraphCreatorFactory ..> GraphCreator : creates
VibrationsGraphCreator --|> GraphCreator
class VibrationsGraphCreator {
+graph_type: "vibrations profile"
}
ShaperGraphCreator --|> GraphCreator
class ShaperGraphCreator {
+graph_type: "input shaper"
}
AxesMapGraphCreator --|> GraphCreator
class AxesMapGraphCreator{
+graph_type: "axes map"
}
BeltsGraphCreator --|> GraphCreator
class BeltsGraphCreator{
+graph_type: "belts comparison"
}
StaticGraphCreator --|> GraphCreator
class StaticGraphCreator{
+graph_type: "static frequency"
}
note for GraphCreatorFactory "Uses GraphCreator.registry to find and instantiate the correct GraphCreator subclass"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Frix-x - I've reviewed your changes - here's some feedback:
- The accelerometer lookup and validation logic in both
axes_shaper_calibrationandcompare_belts_responsesis duplicated—consider extracting it into a shared helper function to reduce code repetition and keep behavior consistent. - After the refactor to a composition-based
GraphCreator, double-check that all CLI and plugin flows still calldefine_output_target()beforecreate_graph()to avoid the “Output target not defined” error. - The updated
__all__ingraph_creators/__init__.pynow exposes many classes—verify that only intended public APIs are exported, and move any internal helpers out of the public interface if needed.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
[resonance_tester].Summary by Sourcery
Refactor graph creation architecture for better modularity and extensibility, fix CLI CSV handling, add manual accelerometer chip selection, and improve belt test reliability.
New Features:
Bug Fixes:
Enhancements:
computationsandplotterspackagesDocumentation: