search

mitch7391 / homebridge-cmd4-AdvantageAir

Catered shell script to integrate air conditioner control units by Advantage Air into HomeKit using the plug-in homebridge-cmd4.
MIT License
38 stars 5 forks source link

[Pull Request] Ability to activate/deactivate diagnostic log ++ #60

Closed uswong closed 2 years ago

uswong commented 2 years ago

name: Ability to activate/deactivate diagnostic log ++ about: Resolve an issue and add an improvement to homebridge-cmd4-AdvantageAir. title: "[Pull Request] Ability to activate/deactivate diagnostic log ++" labels: pull-request assignees: mitch7391

In order to minimize "write" events to the temporary directory, the diagnostic logging is deactivated by default. However, it can be re-activated, if required, via a suffix "-debug" to the IP address in the config file. Moreover, the fresh getSystemData is checked and only write to disk if it is different from the cached myAirData.txt.

Some Linux distros do not have "/tmp" as temporary directory but has $TMPDIR defined and is pointing to a different directory. In this version we will use $TMPDIR as the default temporary directory if defined, otherwise we will use "/tmp".

Is your pull request related to a problem or a new feature? Please describe:

  1. Enhancement/new feature: To minimize "write" events to the disk.

  2. Issue: AdvAir.sh fails with writing to "/tmp" denied in some Linux distros.

Describe the solution you'd have implemented:

  1. To minimize write events to the disk by (1) deactivating the diagnostic logging by default but can be activated, if required, by adding a suffix -debug to the IP address in the config file. (2) checking the fresh getSystemData and only write to the disk if it is different from the cached myAirData.txt.

  2. To resolve the writing to "/tmp" denied issue: The issue was due to the absent of "/tmp" in some Linux distros but have $TMPDIR defined pointing to a different directory. The possible solution is to use $TMPDIR as temporary directory if defined, otherwise use "/tmp".

Do your changes pass local testing:

Additional context:

All enhancements/new features are under the hook changes, no user action is required.

However, if any diagnostic log is desired, activate it by adding a suffix -debug to the IP address in the config file as shown below:

image

An additional Unit Test was also added to test this -debug suffix.

ztalbot2000 commented 2 years ago

For debug, you might have been able to trigger of the debug flag in Cmd4 instead of creating another. Just a thought. This works as well.

John

On Wed, May 11, 2022 at 9:59 AM uswong @.***> wrote:

name: Ability to activate/deactivate diagnostic log ++ about: Resolve an issue and add an improvement to homebridge-cmd4-AdvantageAir. title: "[Pull Request] Ability to activate/deactivate diagnostic log ++" labels: pull-request assignees: mitch7391

In order to minimize "write" events to the temporary directory, the diagnostic logging is deactivated by default. However, it can be re-activated, if required, via a suffix "-debug" to the IP address in the config file. Moreover, the fresh getSystemData is checked and only write to disk if it is different from the cached myAirData.txt.

Some Linux distros do not have "/tmp" as temporary directory but has $TMPDIR defined and is pointing to a different directory. In this version we will use $TMPDIR as the default temporary directory if defined, otherwise we will use "/tmp".

Is your pull request related to a problem or a new feature? Please describe:

1.

Enhancement/new feature: To minimize "write" events to the disk. 2.

Issue: AdvAir.sh fails with writing to "/tmp" denied in some Linux distros.

Describe the solution you'd have implemented:

1.

To minimize write events to the disk by (1) deactivating the diagnostic logging by default but can be activated, if required, by adding a suffix -debug to the IP address in the config file. (2) checking the fresh getSystemData and only write to the disk if it is different from the cached myAirData.txt. 2.

To resolve the writing to "/tmp" denied issue: The issue was due to the absent of "/tmp" in some Linux distros but have $TMPDIR defined pointing to a different directory. The possible solution is to use $TMPDIR as temporary directory if defined, otherwise use "/tmp".

Do your changes pass local testing:

  • Yes, passed shellcheck, all unit tests and has been running flawlessly for couple of days on my E-zone system and JohnW's MyPlace system.
  • No

Additional context:

To active the diagnostic logging, add a suffix `-debug' to the IP address in the config file as shown below:

[image: image] https://user-images.githubusercontent.com/96530237/167862733-44ab38b5-fc09-44af-a4c1-21b2bdecf8d2.png

You can view, comment on, or merge this pull request online at:

https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/60 Commit Summary

File Changes

(29 files https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/60/files)

Patch Links:

- https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/60.patch

https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/60.diff

— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/60, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCXZFAQC7DMWXOC2BZFDVJO4MNANCNFSM5VVBK2MA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

uswong commented 2 years ago

For debug, you might have been able to trigger of the debug flag in Cmd4 instead of creating another. Just a thought. This works as well.

The -debug suffix to the IP address is for the activation and deactivation of the additional general logging we coded in the AdvAir.sh during the development phase for MyPlace Extras. General users do not need them but thought that it would be good to have the option to activate it just in case if they have any issue and we can check what is going on.

mitch7391 commented 2 years ago

@uswong just catching up on this, looks like I should hold off on the merge at this point..?

uswong commented 2 years ago

Hi Mitch, you can go ahead to merge the PR. The AdvAir.sh passed all unit tests with flying colors.

image

The issue is the test on homebridge-ui which passed all tests when run in Mac but have some errors when run in RPi. Apparently node_modules is platform dependant as stated by John in his server.js.

// node_modules is platform dependant. We need to figure this out // in order for testing to work on all platforms

mitch7391 commented 2 years ago

Thanks @uswong and sorry for the hold up on my end!