TeamPiccolo / piccolo3-server

the piccolo3 server
2 stars 3 forks source link

Serial control 2 #10

Closed Murray2015 closed 3 years ago

Murray2015 commented 3 years ago

Main headlines: have made 3 extra classes to hold current, voltage and fan sensors.

Questions

  1. I've created one serial connection class and am handing it into each sensor class. I think this is the only way that the lock will work to stop different classes opening and closing the serial port. Is this a good idea?
  2. On what is now line 353 you've got self.temperatures_sensors... . I'm not sure if this is a typo and should be accessing the private var self._temper..., or if it works fine as it is as via some other python mechanism I don't know.
mhagdorn commented 3 years ago
  1. yes, that's what you need to do. I guess you could be fancy and use a singleton for it but I think that would make it only more complicated.

I see what you have done. Rather than making the various controls/sensors child classes of your serial class, I would pass the same instance of the serial class to the various sensors. You do that in the ControlBox class. This reflects the relationship more acurately, ie. the FanControl isn't a SerialConnection but has one.

  1. I struggle to find the location. however, if it is where I suspect it is, that wasn't a typo. The idea is that you hide access to the private variable by an accessor. In the example below I ensure that a is always a float
    def __init__(self):
    self._a = None
    @property
    def a(self):
    return self._a
    @a.setter
    def a(self,val):
    self._a = float(v)

    The setter is called when you do something like inst.a=1 and fails as you would expect when you do inst.a='hello' In your case the setter is used to send a command to the serial as well as storing the current val in the private variable. Your case is a bit more complicated because you are calling a coroutine. I would expect that

    @a.setter
    async def a(self,val):
    await something

    and then

    await inst.a = 1

    works

mhagdorn commented 3 years ago

ah, I see you realised that in your second patch. Excellent.

BTW, do you send strings to serial or shouldn't that be bytes? Strings are unicode