modules: battery: Add battery percentage request/response events #19

simensrostad · 2025-01-13T17:39:51Z

Add battery percentage request/response events
Send battery percentage to the transport module
Forward battery percentage to nRF Cloud

app/src/common/message_channel.h

app/src/modules/battery/battery.c

tests/module/battery/src/redef.c

tests/module/battery/src/redef.h

github-actions · 2025-01-15T09:10:49Z

app/src/common/message_channel.h

@@ -53,6 +53,8 @@ struct payload {

 #define MSG_TO_PAYLOAD(_msg) ((struct payload *)_msg)

+/** NETWORK_MODULE */


The /** NETWORK_MODULE */ comment seems to be misplaced or not needed here. If it's meant to be a section divider, consider using a more descriptive comment or removing it if it doesn't serve a specific purpose.

github-actions · 2025-01-15T09:10:49Z

app/src/common/message_channel.h

+	enum battery_msg_type type;
+
+	/** Contains the current charge of the battery in percentage. */
+	double percentage;


Consider using a more precise data type for the battery percentage. A double type might be an overkill for this scenario. A uint8_t (0-100) could be sufficient to represent the battery percentage.

github-actions · 2025-01-15T09:10:50Z

app/src/modules/battery/battery.c

-#define MAX_MSG_SIZE \
-	(MAX(sizeof(enum trigger_type), \
-		(MAX(sizeof(struct network_msg), sizeof(enum time_status)))))
+#define MAX_MSG_SIZE sizeof(struct battery_msg)

 BUILD_ASSERT(CONFIG_APP_BATTERY_WATCHDOG_TIMEOUT_SECONDS >


Please ensure proper indentation for better readability.

github-actions · 2025-01-15T09:10:50Z

app/src/modules/battery/battery.c

-			CONFIG_APP_BATTERY_EXEC_TIME_SECONDS_MAX,
-			"Watchdog timeout must be greater than maximum execution time");
+	     CONFIG_APP_BATTERY_EXEC_TIME_SECONDS_MAX,
+	     "Watchdog timeout must be greater than maximum execution time");


Please ensure proper indentation for better readability.

are you sure about this?

github-actions · 2025-01-15T09:10:50Z

app/src/modules/battery/battery.c

@@ -50,20 +46,17 @@
 static const struct device *charger = DEVICE_DT_GET(DT_NODELABEL(npm1300_charger));

 /* Forward declarations */
-static struct s_object s_obj;
 static int charger_read_sensors(float *voltage, float *current, float *temp, int32_t *chg_status);
 static void sample(int64_t *ref_time);

 /* State machine */



The variable s_obj was removed but it's not clear if it was unused or if its usage was removed in other parts of the code. Please ensure that this variable is not used elsewhere in the code to avoid undefined behavior.

github-actions · 2025-01-15T09:10:53Z

tests/module/battery/src/redef.c

+	return 0;
+}
+
+static int channel_get(const struct device *dev, enum sensor_channel chan)


The channel_get function currently does not perform any operation and returns 0. Please implement the functionality or remove it if it's not needed.

github-actions · 2025-01-15T09:10:53Z

tests/module/battery/src/redef.c

+	.channel_get = &channel_get,
+};
+
+struct device_state state = {


The state object is globally defined and can be accessed from anywhere in the code. If it's not intended, consider making it static or moving it inside a function.

github-actions · 2025-01-15T09:10:53Z

tests/module/battery/src/redef.c

+	.init_res = 0U,
+};
+
+struct device mock_charger_device = {


The mock_charger_device object is globally defined and can be accessed from anywhere in the code. If it's not intended, consider making it static or moving it inside a function.

github-actions · 2025-01-15T09:10:53Z

tests/module/battery/src/redef.h

+
+#include <zephyr/device.h>
+
+extern struct device mock_charger_device;


Consider using const qualifier for mock_charger_device if it's not intended to be modified.

github-actions · 2025-01-15T09:10:53Z

tests/module/battery/src/redef.h

+
+extern struct device mock_charger_device;
+
+#undef DEVICE_DT_GET


Redefining DEVICE_DT_GET macro could lead to unexpected behavior in other parts of the codebase that use this macro. Consider using a different approach or ensure this redefinition doesn't affect other modules.

- Add battery percentage request/response events - Send battery percentage to the transport module - Forward battery percentage to nRF Cloud - Add test for battery module - Update test for trigger module to handle the new battery percentage sample event Signed-off-by: Simen S. Røstad <simen.rostad@nordicsemi.no>

github-actions · 2025-01-15T09:37:28Z

app/src/common/message_channel.c

+		 NULL,
+		 NULL,
+		 ZBUS_OBSERVERS_EMPTY,
+		 ZBUS_MSG_INIT(0)


It seems like you're initializing a message with 0. It would be better to use a defined constant instead of a magic number to improve code readability and maintainability.

github-actions · 2025-01-15T09:37:28Z

app/src/common/message_channel.h

@@ -53,6 +53,8 @@ struct payload {

 #define MSG_TO_PAYLOAD(_msg) ((struct payload *)_msg)

+/** NETWORK_MODULE */


The /** NETWORK_MODULE */ comment seems to be misplaced or not needed here. If it's meant to be a section header, consider using a more descriptive comment or removing it if it's not providing any additional context or information.

github-actions · 2025-01-15T09:37:29Z

app/src/common/message_channel.h

+	 * .percentage field of the message.
+	 */
+	BATTERY_PERCENTAGE_SAMPLE_RESPONSE = 0x1,
+


Consider using a more precise data type for the battery percentage. A double type might be an overkill for this use case. An int8_t should suffice if the percentage is always between 0 and 100.

github-actions · 2025-01-15T09:37:29Z

app/src/modules/battery/battery.c

-#define MAX_MSG_SIZE \
-	(MAX(sizeof(enum trigger_type), \
-		(MAX(sizeof(struct network_msg), sizeof(enum time_status)))))
+#define MAX_MSG_SIZE sizeof(struct battery_msg)

 BUILD_ASSERT(CONFIG_APP_BATTERY_WATCHDOG_TIMEOUT_SECONDS >


Please ensure proper indentation for better readability.

github-actions · 2025-01-15T09:37:29Z

app/src/modules/battery/battery.c


 BUILD_ASSERT(CONFIG_APP_BATTERY_WATCHDOG_TIMEOUT_SECONDS >
-			CONFIG_APP_BATTERY_EXEC_TIME_SECONDS_MAX,
-			"Watchdog timeout must be greater than maximum execution time");
+	     CONFIG_APP_BATTERY_EXEC_TIME_SECONDS_MAX,


Please ensure proper indentation for better readability.

github-actions · 2025-01-15T09:37:33Z

tests/module/trigger/src/checks.c


 	/* Allow the test thread to sleep so that the DUT's thread is allowed to run. */
 	k_sleep(K_SECONDS(time_in_seconds));

-	err = zbus_sub_wait_msg(&network_subscriber, &chan, &network_msg, K_MSEC(1000));
+	err = zbus_sub_wait_msg(&subscriber, &chan, payload, K_MSEC(1000));


Consider using a constant for the timeout duration K_MSEC(1000) to avoid magic numbers in the code.

github-actions · 2025-01-15T09:37:33Z

tests/module/trigger/src/checks.h

@@ -6,4 +6,6 @@

 void check_network_event(enum network_msg_type expected_network_type);

-void check_no_network_events(uint32_t time_in_seconds);
+void check_battery_event(enum battery_msg_type expected_battery_type);


The function check_no_events is too generic. It would be better to specify what kind of events it checks. Consider renaming it to something more descriptive, like check_no_battery_events.

github-actions · 2025-01-15T09:37:33Z

tests/module/trigger/src/main.c

@@ -82,14 +82,16 @@ void test_init_to_connected_state(void)
 	uint32_t interval =  HOUR_IN_SECONDS / CONFIG_APP_TRIGGER_TIMEOUT_SECONDS;

 	check_network_event(NETWORK_QUALITY_SAMPLE_REQUEST);
+	check_battery_event(BATTERY_PERCENTAGE_SAMPLE_REQUEST);


It seems like you are calling check_battery_event(BATTERY_PERCENTAGE_SAMPLE_REQUEST); twice, once outside the loop and once inside. If this is not intended, please remove the redundant call.

github-actions · 2025-01-15T09:37:33Z

tests/module/trigger/src/main.c


 	for (int i = 0; i < interval; i++) {
 		check_network_event(NETWORK_QUALITY_SAMPLE_REQUEST);
+		check_battery_event(BATTERY_PERCENTAGE_SAMPLE_REQUEST);


As mentioned above, check_battery_event(BATTERY_PERCENTAGE_SAMPLE_REQUEST); is being called twice. Please ensure this is the intended behavior.

github-actions · 2025-01-15T09:37:33Z

tests/module/trigger/src/main.c

 	}

 	/* Cleanup */
 	send_cloud_disconnected();
-	check_no_network_events(7200);
+	check_no_events(7200);


The function check_no_events(7200); is not clear. It would be better if you could rename it to something more descriptive, like check_no_events_for_duration(7200);.

simensrostad force-pushed the battery_module_fixup branch from c84c095 to f52963e Compare January 14, 2025 15:48

github-actions bot reviewed Jan 14, 2025

View reviewed changes

simensrostad force-pushed the battery_module_fixup branch from f52963e to 2128f8b Compare January 15, 2025 09:02

github-actions bot reviewed Jan 15, 2025

View reviewed changes

simensrostad force-pushed the battery_module_fixup branch from 2128f8b to 12f00a4 Compare January 15, 2025 09:20

simensrostad force-pushed the battery_module_fixup branch from 12f00a4 to 40a220f Compare January 15, 2025 09:22

github-actions bot reviewed Jan 15, 2025

View reviewed changes

simensrostad merged commit b589bff into main Jan 15, 2025
2 of 3 checks passed

simensrostad deleted the battery_module_fixup branch January 15, 2025 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modules: battery: Add battery percentage request/response events #19

modules: battery: Add battery percentage request/response events #19

simensrostad commented Jan 13, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

DematteisGiacomo Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

github-actions bot Jan 15, 2025

		@@ -53,6 +53,8 @@ struct payload {

		#define MSG_TO_PAYLOAD(_msg) ((struct payload *)_msg)

		/** NETWORK_MODULE */


		#include <zephyr/device.h>

		extern struct device mock_charger_device;


		extern struct device mock_charger_device;

		#undef DEVICE_DT_GET

modules: battery: Add battery percentage request/response events #19

modules: battery: Add battery percentage request/response events #19

Conversation

simensrostad commented Jan 13, 2025

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

DematteisGiacomo Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot Jan 15, 2025

Choose a reason for hiding this comment