Review: Needs Fixing

Some comments on the code as we are improving it !

Diff comments:

> 
> === renamed file 'openlp/core/lib/projector/pjlink1.py' => 
> 'openlp/core/lib/projector/pjlink.py'
> --- openlp/core/lib/projector/pjlink1.py      2017-07-20 15:31:50 +0000
> +++ openlp/core/lib/projector/pjlink.py       2017-08-06 07:33:29 +0000
> @@ -69,7 +72,407 @@
>  PJLINK_SUFFIX = CR
>  
>  
> -class PJLink(QtNetwork.QTcpSocket):
> +class PJLinkCommands(object):
> +    """
> +    Process replies from PJLink projector.
> +    """
> +
> +    def __init__(self, *args, **kwargs):
> +        """
> +        Setup for the process commands
> +        """
> +        log.debug('PJlinkCommands(args={args} 
> kwargs={kwargs})'.format(args=args, kwargs=kwargs))
> +        super().__init__()
> +        # Map command to function
> +        self.pjlink_functions = {
> +            'AVMT': self.process_avmt,
> +            'CLSS': self.process_clss,
> +            'ERST': self.process_erst,
> +            'INFO': self.process_info,
> +            'INF1': self.process_inf1,
> +            'INF2': self.process_inf2,
> +            'INPT': self.process_inpt,
> +            'INST': self.process_inst,
> +            'LAMP': self.process_lamp,
> +            'NAME': self.process_name,
> +            'PJLINK': self.check_login,
> +            'POWR': self.process_powr,
> +            'SNUM': self.process_snum,
> +            'SVER': self.process_sver,
> +            'RFIL': self.process_rfil,
> +            'RLMP': self.process_rlmp
> +        }
> +
> +    def reset_information(self):
> +        """
> +        Initialize instance variables. Also used to reset projector-specific 
> information to default.
> +        """
> +        log.debug('({ip}) reset_information() connect status is 
> {state}'.format(ip=self.ip, state=self.state()))
> +        self.fan = None  # ERST
> +        self.filter_time = None  # FILT
> +        self.lamp = None  # LAMP
> +        self.mac_adx_received = None  # ACKN
> +        self.manufacturer = None  # INF1
> +        self.model = None  # INF2
> +        self.model_filter = None  # RFIL
> +        self.model_lamp = None  # RLMP
> +        self.mute = None  # AVMT
> +        self.other_info = None  # INFO
> +        self.pjlink_class = PJLINK_CLASS  # Default class
> +        self.pjlink_name = None  # NAME
> +        self.power = S_OFF  # POWR
> +        self.serial_no = None  # SNUM
> +        self.serial_no_received = None
> +        self.sw_version = None  # SVER
> +        self.sw_version_received = None
> +        self.shutter = None  # AVMT
> +        self.source_available = None  # INST
> +        self.source = None  # INPT
> +        # These should be part of PJLink() class, but set here for 
> convenience
> +        if hasattr(self, 'timer'):
> +            log.debug('({ip}): Calling timer.stop()'.format(ip=self.ip))
> +            self.timer.stop()
> +        if hasattr(self, 'socket_timer'):
> +            log.debug('({ip}): Calling 
> socket_timer.stop()'.format(ip=self.ip))
> +            self.socket_timer.stop()
> +        self.send_busy = False
> +        self.send_queue = []
> +
> +    def process_command(self, cmd, data):
> +        """
> +        Verifies any return error code. Calls the appropriate command 
> handler.
> +
> +        :param cmd: Command to process
> +        :param data: Data being processed
> +        """
> +        log.debug('({ip}) Processing command "{cmd}" with data 
> "{data}"'.format(ip=self.ip,
> +                                                                             
>    cmd=cmd,
> +                                                                             
>    data=data))
> +        # Check if we have a future command not available yet
> +        if cmd not in PJLINK_VALID_CMD:
> +            log.error("({ip}) Ignoring command='{cmd}' 
> (Invalid/Unknown)".format(ip=self.ip, cmd=cmd))
> +            return
> +        elif cmd not in self.pjlink_functions:
> +            log.warn("({ip}) Unable to process command='{cmd}' (Future 
> option)".format(ip=self.ip, cmd=cmd))
> +            return
> +        elif data in PJLINK_ERRORS:
> +            # Oops - projector error
> +            log.error('({ip}) Projector returned error 
> "{data}"'.format(ip=self.ip, data=data))
> +            if data.upper() == 'ERRA':

Would not _data = data.upper() be better then test _data.

> +                # Authentication error
> +                self.disconnect_from_host()
> +                self.change_status(E_AUTHENTICATION)
> +                log.debug('({ip}) emitting projectorAuthentication() 
> signal'.format(ip=self.ip))
> +                self.projectorAuthentication.emit(self.name)
> +            elif data.upper() == 'ERR1':
> +                # Undefined command
> +                self.change_status(E_UNDEFINED, '{error}: 
> "{data}"'.format(error=ERROR_MSG[E_UNDEFINED],
> +                                                                           
> data=cmd))
> +            elif data.upper() == 'ERR2':
> +                # Invalid parameter
> +                self.change_status(E_PARAMETER)
> +            elif data.upper() == 'ERR3':
> +                # Projector busy
> +                self.change_status(E_UNAVAILABLE)
> +            elif data.upper() == 'ERR4':
> +                # Projector/display error
> +                self.change_status(E_PROJECTOR)
> +            self.receive_data_signal()
> +            return
> +        # Command succeeded - no extra information
> +        elif data.upper() == 'OK':
> +            log.debug('({ip}) Command returned OK'.format(ip=self.ip))
> +            # A command returned successfully
> +            self.receive_data_signal()
> +            return
> +        # Command checks already passed
> +        log.debug('({ip}) Calling function for {cmd}'.format(ip=self.ip, 
> cmd=cmd))
> +        self.receive_data_signal()
> +        self.pjlink_functions[cmd](data)
> +
> +    def process_avmt(self, data):
> +        """
> +        Process shutter and speaker status. See PJLink specification for 
> format.
> +        Update self.mute (audio) and self.shutter (video shutter).
> +
> +        :param data: Shutter and audio status
> +        """
> +        shutter = self.shutter
> +        mute = self.mute

Could this not be a dictionary look up?

> +        if data == '11':
> +            shutter = True
> +            mute = False
> +        elif data == '21':
> +            shutter = False
> +            mute = True
> +        elif data == '30':
> +            shutter = False
> +            mute = False
> +        elif data == '31':
> +            shutter = True
> +            mute = True
> +        else:
> +            log.warning('({ip}) Unknown shutter response: 
> {data}'.format(ip=self.ip, data=data))
> +        update_icons = shutter != self.shutter

Need Asprin can we have a comment, please!

> +        update_icons = update_icons or mute != self.mute
> +        self.shutter = shutter
> +        self.mute = mute
> +        if update_icons:
> +            self.projectorUpdateIcons.emit()
> +        return
> +
> +    def process_clss(self, data):
> +        """
> +        PJLink class that this projector supports. See PJLink specification 
> for format.
> +        Updates self.class.
> +
> +        :param data: Class that projector supports.
> +        """
> +        # bug 1550891: Projector returns non-standard class response:
> +        #            : Expected: '%1CLSS=1'
> +        #            : Received: '%1CLSS=Class 1'  (Optoma)
> +        #            : Received: '%1CLSS=Version1'  (BenQ)
> +        if len(data) > 1:
> +            log.warn("({ip}) Non-standard CLSS reply: 
> '{data}'".format(ip=self.ip, data=data))
> +            # Due to stupid projectors not following standards (Optoma, BenQ 
> comes to mind),
> +            # AND the different responses that can be received, the 
> semi-permanent way to
> +            # fix the class reply is to just remove all non-digit characters.
> +            try:
> +                clss = re.findall('\d', data)[0]  # Should only be the first 
> match
> +            except IndexError:
> +                log.error("({ip}) No numbers found in class version reply - 
> defaulting to class '1'".format(ip=self.ip))
> +                clss = '1'
> +        elif not data.isdigit():
> +            log.error("({ip}) NAN class version reply - defaulting to class 
> '1'".format(ip=self.ip))
> +            clss = '1'
> +        else:
> +            clss = data
> +        self.pjlink_class = clss
> +        log.debug('({ip}) Setting pjlink_class for this projector to 
> "{data}"'.format(ip=self.ip,
> +                                                                             
>          data=self.pjlink_class))
> +        return
> +
> +    def process_erst(self, data):
> +        """
> +        Error status. See PJLink Specifications for format.
> +        Updates self.projector_errors
> +
> +\        :param data: Error status
> +        """
> +        try:
> +            datacheck = int(data)
> +        except ValueError:
> +            # Bad data - ignore
> +            return
> +        if datacheck == 0:
> +            self.projector_errors = None
> +        else:
> +            self.projector_errors = {}
> +            # Fan
> +            if data[0] != '0':
> +                self.projector_errors[translate('OpenLP.ProjectorPJLink', 
> 'Fan')] = \
> +                    PJLINK_ERST_STATUS[data[0]]
> +            # Lamp
> +            if data[1] != '0':
> +                self.projector_errors[translate('OpenLP.ProjectorPJLink', 
> 'Lamp')] =  \
> +                    PJLINK_ERST_STATUS[data[1]]
> +            # Temp
> +            if data[2] != '0':
> +                self.projector_errors[translate('OpenLP.ProjectorPJLink', 
> 'Temperature')] =  \
> +                    PJLINK_ERST_STATUS[data[2]]
> +            # Cover
> +            if data[3] != '0':
> +                self.projector_errors[translate('OpenLP.ProjectorPJLink', 
> 'Cover')] =  \
> +                    PJLINK_ERST_STATUS[data[3]]
> +            # Filter
> +            if data[4] != '0':
> +                self.projector_errors[translate('OpenLP.ProjectorPJLink', 
> 'Filter')] =  \
> +                    PJLINK_ERST_STATUS[data[4]]
> +            # Other
> +            if data[5] != '0':
> +                self.projector_errors[translate('OpenLP.ProjectorPJLink', 
> 'Other')] =  \
> +                    PJLINK_ERST_STATUS[data[5]]
> +        return
> +
> +    def process_inf1(self, data):
> +        """
> +        Manufacturer name set in projector.
> +        Updates self.manufacturer
> +
> +        :param data: Projector manufacturer
> +        """
> +        self.manufacturer = data
> +        log.debug('({ip}) Setting projector manufacturer data to 
> "{data}"'.format(ip=self.ip, data=self.manufacturer))
> +        return
> +
> +    def process_inf2(self, data):
> +        """
> +        Projector Model set in projector.
> +        Updates self.model.
> +
> +        :param data: Model name
> +        """
> +        self.model = data
> +        log.debug('({ip}) Setting projector model to 
> "{data}"'.format(ip=self.ip, data=self.model))
> +        return
> +
> +    def process_info(self, data):
> +        """
> +        Any extra info set in projector.
> +        Updates self.other_info.
> +
> +        :param data: Projector other info
> +        """
> +        self.other_info = data
> +        log.debug('({ip}) Setting projector other_info to 
> "{data}"'.format(ip=self.ip, data=self.other_info))
> +        return
> +
> +    def process_inpt(self, data):
> +        """
> +        Current source input selected. See PJLink specification for format.
> +        Update self.source
> +
> +        :param data: Currently selected source
> +        """
> +        self.source = data
> +        log.info('({ip}) Setting data source to "{data}"'.format(ip=self.ip, 
> data=self.source))
> +        return
> +
> +    def process_inst(self, data):
> +        """
> +        Available source inputs. See PJLink specification for format.
> +        Updates self.source_available
> +
> +        :param data: Sources list
> +        """
> +        sources = []
> +        check = data.split()
> +        for source in check:
> +            sources.append(source)
> +        sources.sort()
> +        self.source_available = sources
> +        self.projectorUpdateIcons.emit()
> +        log.debug('({ip}) Setting projector sources_available to 
> "{data}"'.format(ip=self.ip,
> +                                                                             
>      data=self.source_available))
> +        return
> +
> +    def process_lamp(self, data):
> +        """
> +        Lamp(s) status. See PJLink Specifications for format.
> +        Data may have more than 1 lamp to process.
> +        Update self.lamp dictionary with lamp status.
> +
> +        :param data: Lamp(s) status.
> +        """
> +        lamps = []
> +        data_dict = data.split()
> +        while data_dict:
> +            try:
> +                fill = {'Hours': int(data_dict[0]), 'On': False if 
> data_dict[1] == '0' else True}
> +            except ValueError:
> +                # In case of invalid entry
> +                log.warning('({ip}) process_lamp(): Invalid data 
> "{data}"'.format(ip=self.ip, data=data))
> +                return
> +            lamps.append(fill)
> +            data_dict.pop(0)  # Remove lamp hours
> +            data_dict.pop(0)  # Remove lamp on/off
> +        self.lamp = lamps
> +        return
> +
> +    def process_name(self, data):
> +        """
> +        Projector name set in projector.
> +        Updates self.pjlink_name
> +
> +        :param data: Projector name
> +        """
> +        self.pjlink_name = data
> +        log.debug('({ip}) Setting projector PJLink name to 
> "{data}"'.format(ip=self.ip, data=self.pjlink_name))
> +        return
> +
> +    def process_powr(self, data):
> +        """
> +        Power status. See PJLink specification for format.
> +        Update self.power with status. Update icons if change from previous 
> setting.
> +
> +        :param data: Power status
> +        """
> +        log.debug('({ip}: Processing POWR command'.format(ip=self.ip))
> +        if data in PJLINK_POWR_STATUS:
> +            power = PJLINK_POWR_STATUS[data]
> +            update_icons = self.power != power
> +            self.power = power
> +            self.change_status(PJLINK_POWR_STATUS[data])
> +            if update_icons:
> +                self.projectorUpdateIcons.emit()
> +                # Update the input sources available
> +                if power == S_ON:
> +                    self.send_command('INST')
> +        else:
> +            # Log unknown status response
> +            log.warning('({ip}) Unknown power response: 
> {data}'.format(ip=self.ip, data=data))
> +        return
> +
> +    def process_rfil(self, data):
> +        """
> +        Process replacement filter type
> +        """
> +        if self.model_filter is None:
> +            self.model_filter = data
> +        else:
> +            log.warn("({ip}) Filter model already set".format(ip=self.ip))
> +            log.warn("({ip}) Saved model: '{old}'".format(ip=self.ip, 
> old=self.model_filter))
> +            log.warn("({ip}) New model: '{new}'".format(ip=self.ip, 
> new=data))
> +
> +    def process_rlmp(self, data):
> +        """
> +        Process replacement lamp type
> +        """
> +        if self.model_lamp is None:
> +            self.model_lamp = data
> +        else:
> +            log.warn("({ip}) Lamp model already set".format(ip=self.ip))
> +            log.warn("({ip}) Saved lamp: '{old}'".format(ip=self.ip, 
> old=self.model_lamp))
> +            log.warn("({ip}) New lamp: '{new}'".format(ip=self.ip, new=data))
> +
> +    def process_snum(self, data):
> +        """
> +        Serial number of projector.
> +
> +        :param data: Serial number from projector.
> +        """
> +        if self.serial_no is None:
> +            log.debug("({ip}) Setting projector serial number to 
> '{data}'".format(ip=self.ip, data=data))
> +            self.serial_no = data
> +            self.db_update = False
> +        else:
> +            # Compare serial numbers and see if we got the same projector
> +            if self.serial_no != data:
> +                log.warn("({ip}) Projector serial number does not match 
> saved serial number".format(ip=self.ip))
> +                log.warn("({ip}) Saved:    '{old}'".format(ip=self.ip, 
> old=self.serial_no))
> +                log.warn("({ip}) Received: '{new}'".format(ip=self.ip, 
> new=data))
> +                log.warn("({ip}) NOT saving serial 
> number".format(ip=self.ip))
> +                self.serial_no_received = data
> +
> +    def process_sver(self, data):
> +        """
> +        Software version of projector
> +        """
> +        if self.sw_version is None:
> +            log.debug("({ip}) Setting projector software version to 
> '{data}'".format(ip=self.ip, data=data))
> +            self.sw_version = data
> +            self.db_update = True
> +        else:
> +            # Compare software version and see if we got the same projector
> +            if self.serial_no != data:
> +                log.warn("({ip}) Projector software version does not match 
> saved software version".format(ip=self.ip))
> +                log.warn("({ip}) Saved:    '{old}'".format(ip=self.ip, 
> old=self.sw_version))
> +                log.warn("({ip}) Received: '{new}'".format(ip=self.ip, 
> new=data))
> +                log.warn("({ip}) NOT saving serial 
> number".format(ip=self.ip))
> +                self.sw_version_received = data
> +
> +
> +class PJLink(PJLinkCommands, QtNetwork.QTcpSocket):
>      """
>      Socket service for connecting to a PJLink-capable projector.
>      """


-- 
https://code.launchpad.net/~alisonken1/openlp/pjlink2g/+merge/328634
Your team OpenLP Core is subscribed to branch lp:openlp.

_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to     : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

Reply via email to