Creating API Functions for psucontrol#3
Conversation
…ich can be called either from cli.py for command-line interfacing, or in external python scripts for integration with FPGA control software
| return 0 | ||
| except requests.exceptions.RequestException as e: | ||
| print(f"Failed to control PSU: {e}", file=sys.stderr) | ||
| return 1 |
There was a problem hiding this comment.
I think this is probably better off as an exception (rather than a "1" return code and a printed message)
There was a problem hiding this comment.
As in, return the exception? Or instead, interrupt the process (i.e., don't catch the error, just let it stop the process)?
There was a problem hiding this comment.
I copied the 0/1 return from the original, so I thought it would be preferred to keep that as the return.
There was a problem hiding this comment.
For a CLI tool, returning an integer (0 on success, 1 on failure) is conventional - but that's UNIX command-line conventions, not Python conventions, so it's only relevant at the very top level (e.g. main).
When structuring library code, we should use the following convention:
- library code uses exceptions for error paths, does not print, and (sparingly) uses logging only where required
- setters (here, "control_power") do not return a value
- getters (here, "status") return some meaningful value in Python-appropriate type (e.g. True/False, float, int, dict, ...)
So for example:
class PSUController():
# simplest case: just let any exceptions escape
def set_power(self, state : bool):
do_thing()
# no return needed - this is a setter, so there's nothing to say if we succeeded
# or, if the underlying exception doesn't make sense in this context, re-raise it
def set_power_alt(self, state : bool):
try:
do_thing()
except SomeException as e:
raise SomeOtherException("this is a more logical exception or string") from e
| timeout=self.timeout | ||
| ) | ||
| response.raise_for_status() | ||
| print(f"PSU output {'enabled' if state else 'disabled'}") |
There was a problem hiding this comment.
In general, if this is library code it should not be printing to stdout. Do you need these prints (are they useful?)
If you do want to emit text, consider using the logging API instead.
I think this is is generic feedback that applies to a couple of places in the PR.
There was a problem hiding this comment.
I think it's useful to have a printout showing that the output has indeed been changed (even if it's just psychological need)...
Would adding file=sys.stderr to the print command be what you mean?
| print(f" Temperature: {data.get('temp', 0):.1f} °C") | ||
| print(f" Fan speed: {data.get('fan_rpm', 0)} RPM") | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
Here is another point that I realized after the pull request that I'd like to change. I would like this to return the JSON output if requested so that I can actually recover the status and then use it to make decisions about e.g., whether to continue a test or not.
…nstead of printing errors. return useful data (e.g., status values). moved flash back to cli.py since it doesnt require a hostname.
…rocesses to main (e.g., API requests). PSU discovery works, but API calls based on requests library not working. this commit is therefore untested and requires follow-up.
|
Hi @Ian-Hendricksen - is this still a WIP? I just noticed it's still open and we should work towards merging or closing it. |
|
Hey Graeme --- I had meant to reach out to you about this recently! The WIP was not the best commit message ever. I was expecting more changes to come following the issues we were seeing with talking to the PSU over multiple connections, but I've been using my version of psucontrol recently and have had no issues (probably because I was the only one using it on our crate). So, my last commit is working and I think we should merge/close this PR. I have the CHORD F-Engine installed at DRAO, so I'd like to use the "official" psucontrol instead of cloning and pip installing my version. Do let me know if there's anything else I can clean up in the code before we finalize things! |
|
Understood - I forgot about this PR (it wasn't the WIP that threw me off - but I am now trying to catch up). Unfortunately you and @BenCoullNeveu both have conflicting PRs open. |
|
My PR is still very much a WIP. I can try to edit my branch to resolve potential conflicts |
|
I've edited my code to include Ian's |
|
Great - that will allow us to merge this PR and drop the other one (and end up with both). Thanks. |
3 participants
I moved some functions from cli.py to a new script api.py which executes the commands of each script, and reformatted those same functions in cli.py to use API calls.
This change allows for both command-line interface control of the PSU along with integration of the PSU into external repositories such that the PSU can be controlled through Python with the new PSUController class in api.py. PSU discovery remains in cli.py, encouraging a user to use the command-line interface to discover their PSU, and then create an object with PSUController for its corresponding hostname.
Command-line interface control and script-based control were both verified before committing changes.