[task_03] Лабораторная 3 выполнена#360
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements Laboratory Work #3, which includes a PID (Proportional-Integral-Derivative) controller simulation system with both linear and nonlinear process models. The implementation includes:
- A discrete PID controller using a recurrent algorithm
- Process models supporting both linear and nonlinear behavior
- Unit tests using Google Test framework
- Comprehensive documentation using Doxygen
Reviewed changes
Copilot reviewed 44 out of 68 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| trunk/ii02816/task_03/src/pid.h | Header file defining the PIDController class with parameters K, T, Td, T0 |
| trunk/ii02816/task_03/src/pid.cpp | Implementation of PID controller calculation, reset, and coefficient retrieval |
| trunk/ii02816/task_03/src/mdl.h | Header file for ProcessModel class supporting linear and nonlinear models |
| trunk/ii02816/task_03/src/mdl.cpp | Implementation of process model equations |
| trunk/ii02816/task_03/src/main_lab3.cpp | Main program demonstrating PID controller with both model types |
| trunk/ii02816/task_03/test/testlab_3.cpp | Comprehensive unit tests for PID controller and process models |
| trunk/ii02816/task_03/src/CMakeLists.txt | CMake configuration for building the project and tests |
| trunk/ii02816/task_03/doc/readme.md | Detailed documentation of the laboratory work |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 68 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
flikyy
force-pushed
the
task_03_ii02816
branch
from
2896a7a to
3598bb8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 69 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 69 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Head branch was pushed to by a user without write access
flikyy
force-pushed
the
task_03_ii02816
branch
from
bff0265 to
5fb648a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 71 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
trunk/ii02816/task_03/src/main_lab3.cpp:1
- The include directive references "plot_utils.h" but this header file is not present in the codebase. This will cause a compilation error.
trunk/ii02816/task_03/src/main_lab3.cpp:1 - The variable
control_signalsis declared but never used in the function. This creates unnecessary memory allocation and should be removed to improve code maintainability.
trunk/ii02816/task_03/src/main_lab3.cpp:1 - The comment mentions increasing the number of steps, but it's unclear what the original value was or why 100 steps was chosen. The comment should explain the rationale for this specific value.
trunk/ii02816/task_03/src/main_lab3.cpp:1 - The function
generatePythonPlotScript()is called but never defined in the visible code. This will result in a linker error. Additionally, the generated script would be named "plot_results.py" based on line 77, but line 72 tries to execute "plot_pid_results.py".
trunk/ii02816/task_03/src/CMakeLists.txt:1 - The executable target is named "your_target" which is a placeholder name and should be renamed to something more descriptive like "lab3_pid_visualizer" or similar.
trunk/ii02816/task_03/src/CMakeLists.txt:1 - The CMakeLists.txt references "plot_utils.cpp" which does not exist in the provided codebase. This will cause a build failure.
trunk/ii02816/task_03/doc/readme.md:1 - The link text shows "https://v1tyokkk.github.io/OTIS-2025/" but the actual URL points to "https://oniisssss.github.io/OTIS-2025/". These should be consistent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 72 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
trunk/ii02816/task_03/src/main_lab3.cpp:1
- The function
generatePythonPlotScript()is defined at line 76 but is also declared at line 71 withinmain(). This creates a duplicate definition. The function at line 76-142 should be moved beforemain()or the declaration should be removed.
trunk/ii02816/task_03/src/main_lab3.cpp:1 - The
ProcessModelis initialized withinitial_valueof 10.0, but later at line 51 it's reset to 0.0 for the nonlinear simulation. The initial value of 10.0 for the linear simulation is inconsistent with the nonlinear simulation starting at 0.0, which may produce misleading comparison results.
trunk/ii02816/task_03/src/main_lab3.cpp:1 - The comment on line 45 states "Увеличим количество шагов" (increase number of steps) suggesting this was changed from a smaller value. The magic number 100 should be defined as a named constant to improve maintainability and clarity.
trunk/ii02816/task_03/src/main_lab3.cpp:1 - The variable
control_signalsis created and populated but never used. This suggests incomplete implementation or dead code that should be removed.
trunk/ii02816/task_03/doc/readme.md:1 - The documentation link text shows one URL but links to a different URL. The link text is
https://v1tyokkk.github.io/OTIS-2025/but the actual link target ishttps://oniisssss.github.io/OTIS-2025/. These should match.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
flikyy
force-pushed
the
task_03_ii02816
branch
from
e3ccc96 to
7fdbd6a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 72 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 72 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
flikyy
force-pushed
the
task_03_ii02816
branch
from
a68769d to
1d40609
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 72 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ProcessModel process(model_params, 10.0); | ||
|
|
||
| double K = 0.8; | ||
| double T = 4.0; | ||
| double Td = 0.05; | ||
| double T0 = 1.0; | ||
|
|
||
| PIDController pid(K, T, Td, T0); | ||
| std::vector<double> setpoints(100, 20.0); // Увеличим количество шагов | ||
|
|
||
| // Симуляция линейной модели | ||
| auto linear_results = simulateSystem(pid, process, setpoints, false); | ||
|
|
||
| pid.reset(); | ||
| process.setInitialValue(0.0); |
There was a problem hiding this comment.
The initial value is hardcoded as 10.0 in main, but then set to 0.0 for the linear model simulation. This inconsistency could lead to unexpected behavior. The initial value should be consistent or clearly documented why it differs.
| std::vector<double> control_signals; // Сохраняем управляющие воздействия | ||
|
|
||
| for (double setpoint : setpoints) { | ||
| double current_value = (results.empty()) ? 0.0 : results.back(); | ||
| double control_signal = pid.calculate(setpoint, current_value); | ||
| control_signals.push_back(control_signal); |
There was a problem hiding this comment.
The variable control_signals is declared and populated but never used. This creates unnecessary memory overhead and makes the code confusing. Consider removing this unused variable.
| @@ -0,0 +1,190 @@ | |||
| /** | |||
| * @file test.cpp | |||
flikyy
force-pushed
the
task_03_ii02816
branch
from
ffcd10b to
9446830
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 72 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| void generatePythonPlotScript() { | ||
| std::cout << "Running Python visualization..." << std::endl; | ||
| system("python3 plot_pid_results.py"); |
There was a problem hiding this comment.
The function uses the system() call to execute a Python script, which poses a security risk. This can lead to command injection vulnerabilities if the executed command or file path is not properly controlled. Consider using safer alternatives or ensuring the Python script path is validated.
flikyy
force-pushed
the
task_03_ii02816
branch
from
c4878d7 to
6c33876
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 72 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public: | ||
| PIDRegulator(double P_gain, double I_time, double D_time, double sample_time); | ||
| double update(double target, double measurement); |
There was a problem hiding this comment.
The method is named update in the header, but the tests and main program call it as calculate. This naming inconsistency will cause compilation errors. The method name should be consistent across all files.
| public: | ||
| PIDRegulator(double P_gain, double I_time, double D_time, double sample_time); | ||
| double update(double target, double measurement); | ||
| void clear(); |
There was a problem hiding this comment.
The method is named clear in the header, but the tests and main program call it as reset. This naming inconsistency will cause compilation errors. The method name should be consistent across all files.
| /** | ||
| * @file mdl.cpp | ||
| * @brief Implementation of process model methods for thermal object simulation | ||
| * @author Litvinchuk I.M. | ||
| * @date 2025 | ||
| */ | ||
|
|
||
| #include "mdl.h" | ||
| #include <stdexcept> | ||
| #include <cmath> | ||
|
|
||
| /** | ||
| * @brief Constructs a new Process Model object | ||
| * | ||
| * Initializes the thermal object model with given parameters and starting condition. | ||
| * The model represents a physical system with thermal inertia characteristics. | ||
| * | ||
| * @param params Vector containing 4 model coefficients: | ||
| * [a1, a2, b1, b2] where: | ||
| * - a1: Linear state coefficient | ||
| * - a2: Nonlinear damping coefficient | ||
| * - b1: Linear input gain | ||
| * - b2: Nonlinear sinusoidal input gain | ||
| * @param initial_value Starting temperature or output value of the system | ||
| * | ||
| * @throw std::invalid_argument If insufficient parameters provided (less than 4) | ||
| * | ||
| * @note The model assumes normalized values where 0.0 represents minimum | ||
| * and 1.0 represents maximum operational range | ||
| */ | ||
| ProcessModel::ProcessModel(const std::vector<double>& params, double initial_value) | ||
| : prev_value(initial_value) { | ||
|
|
||
| // Validate input parameters | ||
| if (params.size() < 4) { | ||
| throw std::invalid_argument("Process model requires exactly 4 parameters: " | ||
| "linear_coeff, nonlinear_coeff, input_gain, sin_gain"); | ||
| } | ||
|
|
||
| // Copy validated parameters | ||
| this->params = params; | ||
|
|
||
| // Initialize internal state | ||
| this->prev_value = initial_value; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Computes linear model response | ||
| * | ||
| * Implements first-order linear difference equation representing | ||
| * simplified thermal dynamics: | ||
| * y[k] = a₁·y[k-1] + b₁·u[k] | ||
| * | ||
| * This model assumes linear relationship between current output, | ||
| * previous output, and current input without nonlinear effects. | ||
| * | ||
| * @param input Control signal or heat input at current time step | ||
| * @return double Current system output (temperature) | ||
| * | ||
| * @warning Model may exhibit unrealistic behavior for large inputs | ||
| * due to lack of saturation effects | ||
| */ | ||
| double ProcessModel::linearModel(double input) { | ||
| // Linear dynamics calculation | ||
| double output = params[0] * prev_value // State feedback | ||
| + params[1] * input; // Input contribution | ||
|
|
||
| // Update internal state for next iteration | ||
| prev_value = output; | ||
|
|
||
| return output; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Computes nonlinear model response with thermal effects | ||
| * | ||
| * Implements enhanced model capturing nonlinear thermal phenomena: | ||
| * y[k] = a₁·y[k-1] - a₂·y[k-1]² + b₁·u[k] + b₂·sin(u[k]) | ||
| * | ||
| * The model includes: | ||
| * 1. Quadratic damping term representing heat loss non-linearity | ||
| * 2. Sinusoidal input coupling for periodic excitation effects | ||
| * | ||
| * @param input Control signal or heat input at current time step | ||
| * @return double Current system output with nonlinear effects | ||
| * | ||
| * @note The sinusoidal term models alternating heat transfer effects | ||
| * common in certain thermal systems | ||
| */ | ||
| double ProcessModel::nonlinearModel(double input) { | ||
| // Calculate base linear response | ||
| double linear_component = params[0] * prev_value | ||
| + params[2] * input; | ||
|
|
||
| // Add nonlinear effects | ||
| double nonlinear_damping = params[1] * prev_value * prev_value; // Quadratic loss | ||
| double periodic_effect = params[3] * std::sin(input); // Sinusoidal coupling | ||
|
|
||
| // Combine all components | ||
| double output = linear_component | ||
| - nonlinear_damping | ||
| + periodic_effect; | ||
|
|
||
| // Update internal state | ||
| prev_value = output; | ||
|
|
||
| return output; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Resets the model to specified initial condition | ||
| * | ||
| * Allows re-initialization of the model state without reconstructing | ||
| * the object. Useful for comparing multiple simulation runs or | ||
| * switching between different operating points. | ||
| * | ||
| * @param value New initial value for the system state | ||
| * | ||
| * @see ProcessModel::ProcessModel() for constructor initialization | ||
| * | ||
| * @note Resetting preserves model parameters, only changes state | ||
| */ | ||
| void ProcessModel::setInitialValue(double value) { | ||
| prev_value = value; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Retrieves current model parameters | ||
| * | ||
| * Provides read-only access to the model coefficients for | ||
| * diagnostic or analysis purposes. | ||
| * | ||
| * @return const std::vector<double>& Reference to model parameters | ||
| * | ||
| * @warning Modifying returned vector directly may cause undefined behavior | ||
| */ | ||
|
|
||
| const std::vector<double>& ProcessModel::getParameters() const { | ||
| return params; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Retrieves current model state | ||
| * | ||
| * Returns the most recent output value, representing the current | ||
| * state of the simulated thermal system. | ||
| * | ||
| * @return double Current output value (last computed) | ||
| * | ||
| * @note This represents y[k-1] at the beginning of next calculation | ||
| */ | ||
| double ProcessModel::getCurrentState() const { | ||
| return prev_value; | ||
| } No newline at end of file |
There was a problem hiding this comment.
The ProcessModel class is missing implementations for setParameters(), getParameters(), and getPreviousState() methods that are declared in the header file. These missing implementations will cause linker errors.
| ProcessModel::ProcessModel(const std::vector<double>& params, double initial_value) | ||
| : prev_value(initial_value) { | ||
|
|
||
| // Validate input parameters | ||
| if (params.size() < 4) { | ||
| throw std::invalid_argument("Process model requires exactly 4 parameters: " | ||
| "linear_coeff, nonlinear_coeff, input_gain, sin_gain"); | ||
| } | ||
|
|
||
| // Copy validated parameters | ||
| this->params = params; | ||
|
|
||
| // Initialize internal state | ||
| this->prev_value = initial_value; | ||
| } |
There was a problem hiding this comment.
The initial value for ProcessModel is set incorrectly. In the constructor, prev_value is initialized using the initializer list on line 32, but then it's assigned again on line 44. The first initialization is redundant and the member variable current_value is not properly initialized.
|
|
||
| #include <array> | ||
|
|
||
| class PIDRegulator { |
There was a problem hiding this comment.
The class name in the header file is PIDRegulator, but the tests and main program use PIDController. This naming inconsistency will cause compilation errors. The class should be consistently named across all files.
flikyy
force-pushed
the
task_03_ii02816
branch
from
5ecf99d to
2bc134b
Compare
flikyy
force-pushed
the
task_03_ii02816
branch
from
300a43c to
6f070a4
Compare
No description provided.