Gustavodev #15
Gustavodev #15
Conversation
…and made them functions.
| @@ -0,0 +1,163 @@ | |||
| #Copy of ZXY6005s library. | |||
There was a problem hiding this comment.
This file seems to contain all of the same changes already seen in PR #14. Is this accurate?
If so, then we can omit it from this PR.
| #Main program. Objects declared here. Shell built. | ||
| def main(): | ||
| parser = ArgumentParser() | ||
| parser.add_argument('-l', '--arduino-location', help='Location to Arduino. ') |
There was a problem hiding this comment.
I would recommend the verbiage: arduino-device here and in similar situations.
| if (incomingByte != 0){ | ||
| //Shutdown | ||
| if (incomingByte == 97){ | ||
| digitalWrite(xina,LOW); |
There was a problem hiding this comment.
For this and similar situations. I would recommend abstracting this to either a static function or some kind of variable-fed function such as:
static void write(const byte[] write_bytestring, byte*& result) {
// Conditionally set bits based on the bytestring here
// load result so we can verify the bits were set
}This can help separate code so it improves readability and leave a hook for unit-testing.
| return time_step, mag_array #temp_array, mag_array | ||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Is this file intended to be the entry point? If so, why here and not __main__ ?
| // put your main code here, to run repeatedly: | ||
|
|
||
| // reply only when you receive data: | ||
| if (Serial.available() > 0) { |
There was a problem hiding this comment.
While single-char commands are technically fine, it might be helpful to future you, ( as well as any future developers), to actually expand these to human-readable command strings.
This would lend itself to opening up a telnet client to the arduino for manual testing which allows you to develop this apart from the python client.
| self.write_message(msg) | ||
| return self.read_message() | ||
|
|
||
| def create_command(self, command): |
There was a problem hiding this comment.
Excellent that you've separated this out from the actual writing of the serial channel.
Please write unit tests around this function asserting it generates the correct things, (according to your Arduino firmware), or otherwise fails safely.
| msg = f'A{command}\n'.encode() | ||
| return msg | ||
|
|
||
| #definitions for function that help operate the commands. |
There was a problem hiding this comment.
Are these intended to be private functions? If so, add __ to the front of them.
Example:
def __set_positive_x() ->str:Also, please lowercase x/y/z in the names since you are using snake-case.
| print(self.psu.firmware_version(arg[0].upper())) | ||
|
|
||
| #Help message for firmware function. | ||
| def help_firmware(self): |
There was a problem hiding this comment.
If we are going the route of an interactive shell for controlling the cage, I recommend using argparse to handle all of the parsing-related logic.
| def main(): | ||
| parser = ArgumentParser() | ||
| parser.add_argument('-l', '--arduino-location', help='Location to Arduino. ') | ||
| parser.add_argument('-m', '--mock', action = "store_true") |
There was a problem hiding this comment.
Is this mock-mode just for development purposes?
Or is there another configuration mode needed?
If just for development, I would recommend using socat on linux to setup a virtual serial device instead. See man pages.
Example:
socat -d -d pty,raw,echo=1 pty,raw,echo=1Which opens a fake serial device at /dev/pts/N where N is whatever next serial number is available.
This is preferred to adding mock code to your application so you can keep the buisness logic clean and free of baked-in test code.
| @@ -0,0 +1,13 @@ | |||
| from zxy6005s import ZXY6005s | |||
There was a problem hiding this comment.
Is this script intended for manual-regression testing?
If so, please move it and similar scripts to a scripts/ subdir,
|
@andrewgreenberg and I decided it would be best to merge this work as-is and continue building on it. I am merging without @dmitri-mcguckin's requirements (even though I agree with them) so we can move forward. |
|
@andrewgreenberg @ashermancinelli I would strongly insist that this MR got redirected to devel instead, or simply fork the One of the reasons the HHC repo is so thrashed right now is because we've been blindly allowing merges that- while functionally partially-complete still need cleanup and ample testing- so |
|
@dmitri-mcguckin @andrewgreenberg Andrew encouraged me to merge that student's PR tonight. If you'd like something different to happen with the repo, you all can let me know once you've agreed on it. I can revisit this work after that happens. |
|
I got enough context from the slack conversations on what you intend to do and am fine with it. I only wanted to keep this PR if the plan was to adapt and clean what exists. Now that this PR is committed to the tree, I simply recommend wiping away main sooner rather than later, (via a new commit, no squashing), so we don't have any artifact code. That way you can use what's in the commit tree if you still need a reference. Then find a nice python project template to use somewhere. I have one I made for my personal python projects if you're interested. Can link when I'm not on mobile. Just curious, do you need any of the other branches as well? If not- I can cull those as well. Almost time for spring cleaning. |
Created a shell for the HC that controls HC power supplies and the H-Bridges w/ a Arduino Nano.