Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protectli vp2420 1.2.1 rc6 #707

Closed
wants to merge 28 commits into from
Closed

Conversation

matmacieje
Copy link

Platform-specific changes from v1.2.1-rc6 testing on vp2420.

More CPU-related options added to platform config,
WiFi configuration switched to Qualcomm Atheros on both OSes.

Signed-off-by: Mateusz Maciejewski <[email protected]>
Interval argument (second one) with default value 'None' added to
Write Bare Into Terminal keyword, for compatibility with:
https://github.com/3mdeb/robotframework fork.

Signed-off-by: Mateusz Maciejewski <[email protected]>
@matmacieje matmacieje requested a review from macpijan March 5, 2025 09:23
keywords.robot Outdated
@@ -1545,3 +1545,7 @@ Should Contain All
FOR ${substring} IN @{substrings}
Should Contain ${string} ${substring}
END

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matmacieje AFAIK, we can't have Power On = Power On Default in our main keywords file, it's gonna break things for some other platforms.

The approach is that the Power On keyword is defined in the generic platform-configs/include/*-common.robot files for each set of platforms, which are included in the model-specific configs that inherit from them.

If you were getting an error about Power On being ambiguous/missing, something went wrong in the chain of includes from protectli-common through protectli-pro down to the vp2430 config -- either something's missing, or the keyword occurs twice.

So the right fix should probably only happen in the platform-configs folder, please rework this one

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 58f57b7

@filipleple
Copy link
Member

filipleple commented Mar 5, 2025

Also, we generally like to satisfy pre-commit before marking things as ready for review. If your workstation setup went according to plan, it should work like:

pre-commit install
pre-commit run --all-files

called from the OSFV repo.

@@ -256,9 +256,9 @@ Write Bare Into Terminal
...
... === Effects ===
... The ``${text}`` is written to the terminal
[Arguments] ${text}
[Arguments] ${text} ${interval}=None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the interval still needed somewhere? I recall only PC Engines apu platforms with SeaBIOS needed that in iPXE.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reason: dasharo-compatibility/network-boot-utilities.robot, NBT005.001 and NBT007.001; without interval, tests are failing on partial command input. Tested with Write Bare from official robot framework Telnet library.

matmacieje and others added 21 commits March 5, 2025 15:42
Signed-off-by: Mateusz Maciejewski <[email protected]>
Signed-off-by: Filip Gołaś <[email protected]>

lib/sensors.robot: Aesthetical refactor

Signed-off-by: Filip Gołaś <[email protected]>

Make use of lib/sensors platform config vars

Signed-off-by: Filip Gołaś <[email protected]>

lib/sensors/robot: Use triple quotes for string comparisons

Signed-off-by: Filip Gołaś <[email protected]>

lib/sensors: import sensor config variables

Signed-off-by: Filip Gołaś <[email protected]>

lib/sensors.robot: Use hwmon path from sensors config file directly

Signed-off-by: Filip Gołaś <[email protected]>
Signed-off-by: Filip Gołaś <[email protected]>

cpu-fan-speed-measure.robot: Use lib/sensors

Signed-off-by: Filip Gołaś <[email protected]>

custom-fan-curve.robot: Use lib/sensors

Signed-off-by: Filip Gołaś <[email protected]>
Signed-off-by: Filip Gołaś <[email protected]>

platform-configs: Move sensor configs to yaml files

Signed-off-by: Filip Gołaś <[email protected]>

lib/sensors: Use new yaml sensor configs

Signed-off-by: Filip Gołaś <[email protected]>

sensors configs: Remove unnecessary lists

Signed-off-by: Filip Gołaś <[email protected]>

platform-configs/sensors curves: Add tolerances

Signed-off-by: Filip Gołaś <[email protected]>

platform-configs/include/sensors: Set default RPMS to always valid

Signed-off-by: Filip Gołaś <[email protected]>

configs sensors rename vp66xx -> vpxxxx

Signed-off-by: Filip Gołaś <[email protected]>

vpxxxx-fan-curve-config: More accurate RPM curves

Signed-off-by: Filip Gołaś <[email protected]>
To work using PWM or RPM depending on measurement of which is available

Signed-off-by: Filip Gołaś <[email protected]>

custom-fan-curve.robot: Use tolerances & continue if exceeded

Temperatures out of tolerances might still be useful or even
acceptable as a pass with manual verification

Signed-off-by: Filip Gołaś <[email protected]>

custom-fan-curve.robot: Move common test loop to local KW

Signed-off-by: Filip Gołaś <[email protected]>

custom-fan-curve.robot: More logs

Signed-off-by: Filip Gołaś <[email protected]>

custom-fan-curve.robot: Fail only on multiple invalid values in row

Signed-off-by: Filip Gołaś <[email protected]>
Weren't tested on any other device

Signed-off-by: Filip Gołaś <[email protected]>
The results can still be verified manually and used to adjust the curves

Signed-off-by: Filip Gołaś <[email protected]>
philipanda and others added 4 commits March 5, 2025 16:32
Dropping the most anomalous results and deciding on
a pass based on % of valid measurements

Signed-off-by: Filip Gołaś <[email protected]>
Signed-off-by: Mateusz Maciejewski <[email protected]>
@matmacieje matmacieje requested review from filipleple and miczyg1 March 5, 2025 15:36
@filipleple
Copy link
Member

@matmacieje by the ~30 extra changes it looks like something didn't work out when rebasing onto develop.

@matmacieje matmacieje closed this Mar 5, 2025
@matmacieje matmacieje deleted the protectli_vp2420_1.2.1_rc6 branch March 5, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants