cr3ative / homebridge-apcaccess

what if apcaccess, but in homebridge??
MIT License
8 stars 4 forks source link

0.2.1: Add temperature sensor and enhance logging #15

Closed cr3ative closed 1 year ago

cr3ative commented 1 year ago

Addresses #13, #14 and #16 hopefully.

Published as homebridge-apcaccess@0.2.1-rc.1

cr3ative commented 1 year ago

@ThisIsQasim I've published 0.2.1-rc-0. Can you give it a try and see if it does what you want?

ThisIsQasim commented 1 year ago

Thanks for taking this up @cr3ative. I just updated to this version and everything seems to work

davidjbradshaw commented 1 year ago

btw this should probably have a catch() on it, as the one on the setInternval() promise won't catch if this errors.

  getLatestJSON() {
    this.client.getStatusJson().then((result) => {
      this.latestJSON = result;
      this.doPolledChecks();
    });
  }
cr3ative commented 1 year ago

btw this should probably have a catch() on it, as the one on the setInternval() promise won't catch if this errors.

  getLatestJSON() {
    this.client.getStatusJson().then((result) => {
      this.latestJSON = result;
      this.doPolledChecks();
    });
  }

Agreed, let's get this one merged then pick up other things together. I'll push some changes shortly to simplify this PR a little and get it published.

cr3ative commented 1 year ago

Okay, I've decided to:

Published as rc 0.2.1: https://www.npmjs.com/package/homebridge-apcaccess/v/0.2.1?activeTab=versions

Tested with actual installation and usage for errorLogsOnly on and off, new logging behaviour, and tested that temperatureSensor appears to be somewhat sane.

Pending me missing an obvious crash, I'll get this published and merged tomorrow.

davidjbradshaw commented 1 year ago

The moment you disconnect power.

TypeError: console.log.debug is not a function
    at APCAccess.doPolledChecks (/var/lib/homebridge/node_modules/homebridge-apcaccess/index.js:148:19)
    at /var/lib/homebridge/node_modules/homebridge-apcaccess/index.js:81:12
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Causes a restart of HomeBridge. Which is a bit odd as other HB plugins use console.log.debug.

cr3ative commented 1 year ago

The moment you disconnect power.

TypeError: console.log.debug is not a function
    at APCAccess.doPolledChecks (/var/lib/homebridge/node_modules/homebridge-apcaccess/index.js:148:19)
    at /var/lib/homebridge/node_modules/homebridge-apcaccess/index.js:81:12
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Causes a restart of HomeBridge. Which is a bit odd as other HB plugins use console.log.debug.

HAHA WERHEY. Should just be console.debug rather than console.log.debug. AUGH. I'll take a look.

davidjbradshaw commented 1 year ago

Really it should be this.log.debug to run it through HB

cr3ative commented 1 year ago

Really it should be this.log.debug to run it through HB

Gotcha, let me push that real quick

davidjbradshaw commented 1 year ago

Also you your adding console.error in the error off state, which will bypass HB

cr3ative commented 1 year ago

Published as homebridge-apcaccess@0.2.2-rc1 for testing again.

cr3ative commented 1 year ago

Also you your adding console.error in the error off state, which will bypass HB

This is the first I'm learning of homebridge's logging, to be honest with you. Suggest a working pass-through?

Change out the line for error: log.error,?

davidjbradshaw commented 1 year ago

My suggestion for this would to extend lib/logUpdate.js

const keys = Object.create(null)

const T = () => true

const logOnlyError = (log) => ({
  ...log,
  info:T,
  update:T,
})

const logMin = (log) => ({
  ...log,
  update: (key, value) => {
    const update = (keys[key] !== value)

    if(update) keys[key] = value

    return log[update ? 'info' : 'debug'](key, value)
  }
})

module.exports = {
  logMin,
  logOnlyError
}
davidjbradshaw commented 1 year ago

If you merge this, I can make another PR.

cr3ative commented 1 year ago

If you merge this, I can make another PR.

Thanks, I appreciate your help with this. I'll merge this PR, leave the package marked as RC and we can work in a new PR.

cr3ative commented 1 year ago

That said, I did have a stab at it and it's up as -rc4 :)

davidjbradshaw commented 1 year ago
this.log = config.errorLogsOnly ? logOnlyError(log) : logMin(log)
cr3ative commented 1 year ago
this.log = config.errorLogsOnly ? logOnlyError(log) : logMin(log)

FANCY MAN WITH HIS FANCY SYNTAX (yes, that's clean, I'll happily take that in a PR)

davidjbradshaw commented 1 year ago

OK let me workout why installing the last version blows up

USER: homebridge
DIR: /var/lib/homebridge
CMD: npm install --save homebridge-apcaccess@0.2.2-rc4

npm ERR! code ENOTEMPTY
npm ERR! syscall rename
npm ERR! path /var/lib/homebridge/node_modules/homebridge-apcaccess
npm ERR! dest /var/lib/homebridge/node_modules/.homebridge-apcaccess-ICyiPAfA
npm ERR! errno -39
npm ERR! ENOTEMPTY: directory not empty, rename '/var/lib/homebridge/node_modules/homebridge-apcaccess' -> '/var/lib/homebridge/node_modules/.homebridge-apcaccess-ICyiPAfA'

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/homebridge/.npm/_logs/2023-02-23T12_12_25_730Z-debug-0.log

Operation failed. Please review log for details.
davidjbradshaw commented 1 year ago
 sudo rm -rf /var/lib/homebridge/node_modules/.homebridge-apcaccess-ICyiPAfA

Back in the game

cr3ative commented 1 year ago
 sudo rm -rf /var/lib/homebridge/node_modules/.homebridge-apcaccess-ICyiPAfA

Back in the game

HECK YEA