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

add commandline tool to check rtc battery status #438

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nhahnetsrot
Copy link

The patch adds the readrtcbattery commandline tool that prints the current status of the rtc battery (1 == faulty, 0 == ok).

@@ -0,0 +1,7 @@
cmake_minimum_required(VERSION 3.2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright header.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

}
// Close driver
printf("Closing Driver\n");
close(fd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indention

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected


// 2nd step: Display error in command line if the device
// file wasn't found and close application
if(fd < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if is not a function, therefore if (...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

int retval = 0; // Return value of ioctl function (-1 = Error)

// 1st step: Open Driver
printf("\nOpening Driver\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too verbose

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed printf statement



// 3rd step: Read out
printf("Process ioctl access to read battery status\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed print statement

@jan-kiszka
Copy link
Collaborator

Commit message should always contain a reason WHY we want this change.

We may also think about who to make this tool discoverable to our users.

Another improvement could be calling this tool from a systemd timer once a week or so and log a system error if a low battery is detected.

@nhahnetsrot
Copy link
Author

Commit message should always contain a reason WHY we want this change.

Ok, i added this

We may also think about who to make this tool discoverable to our users.

What do you mean with that ?

Another improvement could be calling this tool from a systemd timer once a week or so and log a system error if a low battery is detected.

Yes, sounds like a good idea ... shall i do that (ask Volker about that)?
My information was, that somehow this tool is intended to be called from some orcla module and then the battery-warning may show up in some high-level tool / health-gui.

@jan-kiszka
Copy link
Collaborator

General remark: Please fold changes to the patches into the existing ones (interactive rebase), rather than patching on top.

@jan-kiszka
Copy link
Collaborator

We may also think about who to make this tool discoverable to our users.

What do you mean with that ?

Having it mentioned in the README e.g.

Another improvement could be calling this tool from a systemd timer once a week or so and log a system error if a low battery is detected.

Yes, sounds like a good idea ... shall i do that (ask Volker about that)? My information was, that somehow this tool is intended to be called from some orcla module and then the battery-warning may show up in some high-level tool / health-gui.

We don't have those tools included here. This layer is supposed to be free-standing first of all, providing guidance / examples / re-usable bits how to make use of the hardware.

@@ -0,0 +1,54 @@
/**
* Copyright (c) Siemens AG, 2019-2023
Copy link
Collaborator

@BaochengSu BaochengSu May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use 2023 only

@@ -0,0 +1,15 @@
#
# Copyright (c) Siemens AG, 2019-2023
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Comment on lines +25 to +54
int fd; // Device file path
int batteryStatus; // Status of battery
int retval = 0; // Return value of ioctl function (-1 = Error)

// 1st step: Open Driver
fd = open("/dev/rtc", O_RDONLY);

// 2nd step: Display error in command line if the device
// file wasn't found and close application
if (fd < 0) {
printf("Device file not found\n");
return EXIT_FAILURE;
}

// 3rd step: Read out
retval = ioctl(fd, RTC_VL_DATA_INVALID, &batteryStatus);

if (retval == -1) {
// 4th step: Display error in command line if the ioctl function isn't able to access
// the specific function
printf("Error access ioctl function \n");
return EXIT_FAILURE;
} else {
// 5th step: Print battery status
printf("Battery status (1 = Error) %d\n", batteryStatus);
}
close(fd);

return EXIT_SUCCESS;
} // main
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is simple and straight-forward enough to be self-explained, so most of the comments are unnecessary.

// 4th step: Display error in command line if the ioctl function isn't able to access
// the specific function
printf("Error access ioctl function \n");
return EXIT_FAILURE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close(fd) before return error.

@AsuraZeng
Copy link
Contributor

hi

If no battery or voltage is low, using the existing tool hwclock also can show the status.

hwclock 
[  107.495137] rtc-pcf8563 2-0051: low voltage detected, date/time is not reliable.
hwclock: ioctl(RTC_RD_TIME) to /dev/rtc0 to read the time failed: Invalid argument

@@ -45,6 +45,7 @@ IMAGE_INSTALL += " \
libteec1 \
optee-client-dev \
tee-supplicant \
read-rtc-battery \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to test it?

Comment on lines +47 to +50
} else {
// 5th step: Print battery status
printf("Battery status (1 = Error) %d\n", batteryStatus);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to wrap this print to the else branch, since the return in if.

@jan-kiszka
Copy link
Collaborator

hi

If no battery or voltage is low, using the existing tool hwclock also can show the status.

hwclock 
[  107.495137] rtc-pcf8563 2-0051: low voltage detected, date/time is not reliable.
hwclock: ioctl(RTC_RD_TIME) to /dev/rtc0 to read the time failed: Invalid argument

hwclock is apparently not using RTC_VL_READ while only that IOCTL is triggering the readout of the RTC status bit we need.

}

// 3rd step: Read out
retval = ioctl(fd, RTC_VL_DATA_INVALID, &batteryStatus);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jan-kiszka
Copy link
Collaborator

A customer analyzed a problem with the current implementation of RTC_VL_READ in our RTC driver: Every write clears the INVALID bit, and that prevents checking the state after, e.g., a write-back done on NTP adjustment during boot. Unfortunately, he didn't report his findings to the upstream kernel.

So I'm trying to resolve that there, see https://lore.kernel.org/lkml/[email protected]/T/#u. A simple python script to read the ioctl proved it's working on our hardware.

@jan-kiszka
Copy link
Collaborator

Seems the hardware does not permit proper read-out, and the kernel does not allow working around that limitation. I'm not sure if it still makes sense to follow this path, given that Linux has no framework to report the battery state via some userspace service. Users could still evaluate the system boot log for warnings about the battery state.

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

Successfully merging this pull request may close these issues.

4 participants