Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ALARMDEL can't be parsed as a time.Duration #10

Closed
ivorybilled opened this issue Dec 11, 2020 · 2 comments
Closed

ALARMDEL can't be parsed as a time.Duration #10

ivorybilled opened this issue Dec 11, 2020 · 2 comments

Comments

@ivorybilled
Copy link

Hello,

Currently in status.go, we have this bit of code, which parses the ALARMDEL value:

case keyAlarmDel:
		// No alarm delay configured.
		if v == "No alarm" {
			break
		}

s.AlarmDel, err = parse()

this accounts for the ALARMDEL having a value of "No alarm". Otherwise, it parses it as a time.Duration. However, this won't always work because there are two other possible non-duration values, per the apcupsd library's source (apcstatus.c):

case 'T':
         s_write(ups, "ALARMDEL : 30 Seconds\n");
         break;
      case 'L':
         s_write(ups, "ALARMDEL : Low Battery\n");
         break;
      case 'N':
         s_write(ups, "ALARMDEL : No alarm\n");
         break;
      case '0':
         s_write(ups, "ALARMDEL : 5 Seconds\n");
         break;
      default:
         s_write(ups, "ALARMDEL : Always\n");
         break;
      }

So, the two that aren't handled currently are 'Always' and 'Low Battery', with 'Always' meaning that alarms should sound immediately and continuously upon power fail, and 'Low Battery' meaning that the alarm should sound only after power fails and battery becomes low. (source).

So I guess 'Always' could be converted to just a '0' duration, but 'Low Battery' is unclear. So it seems like we'd either need to change the status.AlarmDel value to a string, or would have to come up with some duration number for 'Low Battery'?

Let me know what makes sense and I could test it out and make a PR. This is currently causing errors in a Telegraf plugin (influxdata/telegraf#8521).

@sjwang90
Copy link

@mdlayher Is this project still active? We are willing to expedite this fix and open a PR if it'll get merged into the project in a timely fashion.

@mdlayher
Copy link
Owner

Fixed in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants