5
\$\begingroup\$

I am creating an automated test system (scope bloat - the original intent was a one-off bash automation of a particular test case that has turned into creating a system that will do many other things). I've done a few things similar to this, but somehow never got to using classes; everything was down at the functions level in modules I wrote for past projects.

I'm looking for a simple check/confirmation that I'm approaching this right. It's a communications module intended to connect via telnet to a remote Linux machine, then pull various I2C reads from a device that the Linux machine is connected to.

I want to make sure that I am creating this correctly correctly, and declaring objects correctly so the functions/methods in the class object will perform their individual tasks correctly. I think I'm on the right track but having people a little more experienced take a look couldn't hurt. In particular, I want to make sure I'm setting the class up for instantiation correctly, and declaring self/__init__ in the right way.

Note: the code here worked reasonably well when it was separate functions not gathered under a unifying class structure, but I was mucking about with global declarations which made me nervous, and it took more tweaking than I was happy with.

class Communications: #logs in to the remote as root.
    def __init__(self):
        self.term = telnetlib.open('10.100.100.103')
        print ('logging in now')
        term.read_until(b"ogin: ")
        term.write('root\n')
        time.sleep(3)
        term.read_very_eager()

    #device power-up and power-down
    def node_startup(node):
        if node == 1:
            node_command = 'node_on 1\n'
        elif node == 2:
            node_command = 'node_on 2\n'
        term.write(node_command)
        print (b'powering node', node, 'on\n')

    def node_shutdown(node):
        if node == 1:
            node_command = 'node_off 1\n'
        elif node == 2:
            node_command = 'node_off 2\n'
        term.write(node_command)
        print (b'shutting node', node, ' down')

    #device sample retrieval
    def get_voltage_i2c(bus, device, datapoint, test, node):
        i2c_sample =  term.write('i2cget -y -f %d %d 0x 8b w >> /tmp/%s_%d.txt\n' %(bus, device, test, node))

    def get_current_i2c(bus, device, datapoint, test, node):
        i2c_sample =  term.write('i2cget -y -f %d %d %d %s %d w >> /tmp/%s_%d.txt\n' %(bus, device, datapoint, test, node))


    def get_power_i2c(bus, device, datapoint, test, node):
        i2c_sample =  term.write('i2cget -y -f %d %d %d %s %d w >> /tmp/%s_%d.txt\n' %(bus, device, datapoint, test, node))

    #output-scraper to slurp results from the output files the i2cgets write to. (devnote: incomplete at time of posting)
    def get_sample_results():
        time.sleep(30)
        term.read_very_eager()
        term.write(b'cat ')
\$\endgroup\$
3
\$\begingroup\$

First thing I noticed is you don't seem to be checking if your commands are executed. There's no Exception handling to be found.

Second, avoid the use of magic numbers.

telnetlib.open('10.100.100.103') should be telnetlib.open(TARGET_IP)

time.sleep(3) should be time.sleep(SLEEP_INIT)

Etcetera. Ofcourse, those variables should be declared on the top of your file. Although Python does not support constant by default, there's a workaround if you're interested.

\$\endgroup\$
  • \$\begingroup\$ the magic numbers are in there for the time being, they're on the to-do list for changing out before this beast i'm creating goes into the wild within the company. consider their presence a temporary state due to this being proof of concept level code. i'll make note of the exception handling and start dropping a bit of it into the methods. i've been doing a lot of monitoring with realterm in snoop mode, but you bring up a good point. \$\endgroup\$ – Jiynx Apr 24 '15 at 17:07
3
\$\begingroup\$

Global variable term vs instance variable self.term

Something is off here:

class Communications: #logs in to the remote as root.
    def __init__(self):
        self.term = telnetlib.open('10.100.100.103')
        print ('logging in now')
        term.read_until(b"ogin: ")

You assign to self.term, but then you use term.read_until instead of self.term.read_until.

Note that self.term and term are NOT the same thing. term is a free variable: it's defined in the enclosing code, and it was not passed in as a parameter. self.term is an instance variable of the class.

It would seem that in all your methods you really meant to write self.term instead of just term.

Don't take actions in constructors

The constructor (the __init__ method) logs in without further ado. It's not usual. Usually a constructor's job is to create an object, ready for action but without actually taking any actions.

Undefined variables, parameter validation

In this code, what will happen if node = 3 ?

def node_startup(node):
    if node == 1:
        node_command = 'node_on 1\n'
    elif node == 2:
        node_command = 'node_on 2\n'
    term.write(node_command)
    print (b'powering node', node, 'on\n')

Nothing good. Validate the inputs and handle unexpected values gracefully.

Unused variables

The various get_*_i2c methods all have this form:

def get_something_i2c(bus, device, datapoint, test, node):
    i2c_sample =  term.write('...')

They assign to i2c_sample and then do nothing else. As the variable is unused, the code doesn't really make sense.

Repetitive code

The node_startup and node_shutdown have mostly duplicated logic. It would be better to factor out the duplicated elements to common methods, for example:

def is_valid_node(node):
    return node in (1, 2)

def execute_node_command(node, command, message):
    if not self.is_valid_node(node):
        return
    self.term.write('{} {}\n'.format(command, node))
    print(message, 'node', node)

def node_startup(node):
    self.execute_node_command(node, 'node_on', 'powering on')

def node_shutdown(node):
    self.execute_node_command(node, 'node_off', 'shutting down')

Do similarly for the various get_*_i2c methods.

Naming

Communications is too generic. And its a plural term, suggesting some sort of a collection. Something like NodeManager would seem more intuitive.

\$\endgroup\$
  • \$\begingroup\$ those repetitive i2c commands were actually one single command instead of various other commands, as they're called in a different script i've been tinkering with the idea of making each command explicit in its name what it does. they output strictly elsewhere to a log file that gets called later. there will be a traffic control signal implemented so that the flow script halts if for some reason they don't work. \$\endgroup\$ – Jiynx Apr 25 '15 at 16:35
  • \$\begingroup\$ noted on taking the action in the constructor however, i'll make that its own little section. the intention is that when the class is called in the flow, it auto-opens the communications to the remote box and preps for send/receive. i can bake that into the class in places other than the constructor, i believe. and the codeself.termcode is a really useful note. i had worried about it and turns out i done broke it. that will be updated first thing. \$\endgroup\$ – Jiynx Apr 25 '15 at 16:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.