Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.idea/
.DS_Store
.cache

build/
cmake-build-*/
Expand Down
16 changes: 16 additions & 0 deletions Apps/AHT10Sensor/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
cmake_minimum_required(VERSION 3.20)

include($ENV{IDF_PATH}/tools/cmake/project.cmake)

if (DEFINED ENV{TACTILITY_SDK_PATH})
set(TACTILITY_SDK_PATH $ENV{TACTILITY_SDK_PATH})
else()
set(TACTILITY_SDK_PATH "../../release/TactilitySDK")
message(WARNING "⚠️ TACTILITY_SDK_PATH environment variable is not set, defaulting to ${TACTILITY_SDK_PATH}")
endif()

include("${TACTILITY_SDK_PATH}/TactilitySDK.cmake")
set(EXTRA_COMPONENT_DIRS ${TACTILITY_SDK_PATH})

project(AHT10Sensor)
tactility_project(AHT10Sensor)
6 changes: 6 additions & 0 deletions Apps/AHT10Sensor/main/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
file(GLOB_RECURSE SOURCE_FILES Source/*.c*)

idf_component_register(
SRCS ${SOURCE_FILES}
REQUIRES TactilitySDK driver
)
102 changes: 102 additions & 0 deletions Apps/AHT10Sensor/main/Source/aht10.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#include "aht10.h"

#include <tactility/device.h>
#include <tactility/drivers/i2c_controller.h>
#include <tactility/freertos/freertos.h>

#define AHT10_ADDR 0x38

static struct Device* s_i2c = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Drivers can be used on more than 1 I2C bus, so storing the I2C device globally would break in that scenario.

If a driver needs to store data related to a device, it can do so with device_set_driver_data().

In this case, the I2C controller doesn't need to be stored, as it should be the parent device of our aht10 device. (see my other comment)


/* ------------------------------ */
/* Find I2C controller */
/* ------------------------------ */

static bool find_i2c(struct Device* dev, void* ctx)
{
s_i2c = dev;
return false;
}
Comment on lines +15 to +19
Copy link

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

find_i2c always selects the first I2C controller found.

On hardware with multiple I2C buses (e.g., a bus dedicated to a display and another to sensors), the AHT10 may not be on the first controller discovered. Consider accepting an I2C bus index or identifier via ctx (already available as the callback parameter) to allow callers to specify the intended bus.

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I2C device drivers work is that their parent should be an I2C controller.
The parent is set in the Device instance itself:

/** Represents a piece of hardware */
struct Device {
    /** The name of the device. Valid characters: a-z a-Z 0-9 - _ . */
    const char* name;

    /** The configuration data for the device's driver */
    const void* config;

    /** The parent device that this device belongs to. Can be NULL, but only the root device should have a NULL parent. */
    struct Device* parent;

    /**
     * Internal state managed by the kernel.
     * Device implementers should initialize this to NULL.
     * Do not access or modify directly; use device_* functions.
     */
    struct DeviceInternal* internal;
};

This way, the I2C (child) device driver doesn't need to know directly which bus it belongs to: it can assume that the parent is an I2C controller device.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recently made an I2C device driver that you could look at for reference:
https://github.com/TactilityProject/Tactility/tree/main/Drivers/pi4ioe5v6408-module

(with the caveat that it is only a simple I2C device for now, this specific one should eventually emit a proper GPIO controller, but you can disregard that)

Copy link

Choose a reason for hiding this comment

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

@KenVanHoeylandt, thank you for the clarification! I understand now—in the Tactility framework, I2C device drivers should rely on the parent-child relationship defined in the Device struct rather than enumerating and selecting controllers. The parent field establishes which I2C controller the device belongs to, so the device driver can simply assume its parent is the appropriate I2C controller.

This makes the architecture much cleaner and avoids the need for selection logic. I'll keep this pattern in mind for future reviews of I2C device drivers in this project.

🐰💡


✏️ Learnings added
Learnt from: KenVanHoeylandt
Repo: TactilityProject/TactilityApps PR: 28
File: Apps/AHT10Sensor/main/Source/aht10.c:15-19
Timestamp: 2026-02-26T17:36:39.298Z
Learning: In TactilityProject repositories, I2C device drivers should use the parent-child relationship defined in the `Device` struct. The `parent` field in the Device struct should be set to the appropriate I2C controller device. Device drivers can assume their parent is the correct I2C controller and don't need to enumerate or select from multiple controllers.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: KenVanHoeylandt
Repo: TactilityProject/TactilityApps PR: 21
File: Apps/MystifyDemo/tactility.py:382-383
Timestamp: 2026-02-06T19:33:55.104Z
Learning: In the TactilityProject/TactilityApps repository, for any file named `tactility.py`, only report major issues or worse during code reviews. Skip minor issues, informational notes, and trivial suggestions.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the reference I2C device driver implementation
curl -s https://raw.githubusercontent.com/TactilityProject/Tactility/main/Drivers/pi4ioe5v6408-module/pi4ioe5v6408.c | head -100

Length of output: 14


🏁 Script executed:

#!/bin/bash
# Use GitHub API to explore the Drivers directory
gh api repos/TactilityProject/Tactility/contents/Drivers --jq '.[].name'

Length of output: 314


🏁 Script executed:

#!/bin/bash
# List contents of the pi4ioe5v6408-module directory
gh api repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module --jq '.[].name'

# Fetch the main implementation file
gh api repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/pi4ioe5v6408.c --jq -r '.content' | base64 -d

Length of output: 115


🏁 Script executed:

#!/bin/bash
# List contents of the source directory
gh api repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/source --jq '.[].name'

# Fetch the main C file
gh api repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.c --jq -r '.content' | base64 -d

Length of output: 66


🏁 Script executed:

#!/bin/bash
# Fetch the pi4ioe5v6408.cpp file
gh api repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cpp --jq -r '.content' | base64 -d

echo "---"
echo "Fetching module.cpp:"
echo "---"

# Also fetch module.cpp to see the structure
gh api repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/source/module.cpp --jq -r '.content' | base64 -d

Length of output: 86


🏁 Script executed:

#!/bin/bash
# Fetch raw content directly using GitHub API
curl -s -H "Accept: application/vnd.github.raw" \
  "https://api.github.com/repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cpp" 

echo ""
echo "===================="
echo "module.cpp:"
echo "===================="

curl -s -H "Accept: application/vnd.github.raw" \
  "https://api.github.com/repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/source/module.cpp"

Length of output: 1326


🏁 Script executed:

#!/bin/bash
# Use gh api with authentication to fetch the file
gh api /repos/TactilityProject/Tactility/contents/Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cpp \
  --jq '.content' | python3 -c "import sys, base64; print(base64.b64decode(sys.stdin.read()).decode())"

Length of output: 4173


@KenVanHoeylandt Thank you for the reference! I can see the proper pattern now. In the pi4ioe5v6408 driver, the device uses device_get_parent(device) to get the I2C controller rather than searching for one globally. Each I2C operation then passes the parent to the i2c_controller_* functions:

auto* parent = device_get_parent(device);
return i2c_controller_register8_set(parent, GET_CONFIG(device)->address, ...);

This makes sense architecturally - the parent-child relationship in the Device structure defines which I2C bus the device belongs to, eliminating the need for global discovery.

I can see that the current AHT10 implementation uses a different approach (global search via find_i2c()), but I understand from your explanation that properly structured device drivers rely on the parent relationship instead.


✏️ Learnings added
Learnt from: KenVanHoeylandt
Repo: TactilityProject/TactilityApps PR: 28
File: Apps/AHT10Sensor/main/Source/aht10.c:15-19
Timestamp: 2026-02-26T17:37:36.652Z
Learning: In the TactilityProject repositories, I2C device drivers should use `device_get_parent(device)` to obtain their I2C controller, not search for controllers globally. The parent Device pointer defines which I2C bus the device belongs to. All I2C operations should pass the parent device to `i2c_controller_*` functions. See the pi4ioe5v6408 driver as a reference implementation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: KenVanHoeylandt
Repo: TactilityProject/TactilityApps PR: 21
File: Apps/MystifyDemo/tactility.py:382-383
Timestamp: 2026-02-06T19:33:55.104Z
Learning: In the TactilityProject/TactilityApps repository, for any file named `tactility.py`, only report major issues or worse during code reviews. Skip minor issues, informational notes, and trivial suggestions.


static bool setup_i2c(void)
{
s_i2c = NULL;

device_for_each_of_type(
&I2C_CONTROLLER_TYPE,
NULL,
find_i2c);

if (!s_i2c)
return false;

return device_is_ready(s_i2c);
}

/* ------------------------------ */
/* Init sensor */
/* ------------------------------ */

bool aht10_init(void)
{
if (!setup_i2c())
return false;

uint8_t init_cmd[3] = {0xE1, 0x08, 0x00};

return i2c_controller_write(
s_i2c,
AHT10_ADDR,
init_cmd,
sizeof(init_cmd),
pdMS_TO_TICKS(100))
== ERROR_NONE;
}

/* ------------------------------ */
/* Read measurement */
/* ------------------------------ */

bool aht10_read(float* temperature, float* humidity)
{
if (!s_i2c)
return false;

uint8_t cmd[3] = {0xAC, 0x33, 0x00};

if (i2c_controller_write(
s_i2c,
AHT10_ADDR,
cmd,
sizeof(cmd),
pdMS_TO_TICKS(100)) != ERROR_NONE)
return false;

vTaskDelay(pdMS_TO_TICKS(80));
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

vTaskDelay(80 ms) inside an LVGL timer callback blocks the LVGL task.

The call chain is lv_timer_create(sensor_update)sensor_updateaht10_readvTaskDelay(pdMS_TO_TICKS(80)). LVGL timers fire within the LVGL task; blocking that task for 80 ms every 2 seconds stalls all LVGL rendering and event processing for that window, causing periodic UI freezes.

The recommended fix is to decouple the I2C read from the LVGL thread — either dedicate a FreeRTOS task to sensor reading and share results with the LVGL thread via a mutex-protected struct, or split the read into a two-step state machine (trigger in one timer tick, read in the next after 80 ms has elapsed).


uint8_t data[6];

if (i2c_controller_read(
s_i2c,
AHT10_ADDR,
data,
sizeof(data),
pdMS_TO_TICKS(100)) != ERROR_NONE)
return false;
Comment on lines +77 to +85
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Status byte (data[0]) is never checked before parsing measurement data.

Per the AHT10 datasheet, byte 0 of the response contains the status register:

  • Bit 7 (busy flag) — 1 means measurement is still in progress; data in bytes 1–5 is invalid.
  • Bit 3 (calibration flag) — 0 means the sensor is uncalibrated; readings are unreliable.

Skipping these checks means that if the sensor is still busy (however unlikely after 80 ms) or uncalibrated, corrupt values are silently written to *humidity and *temperature.

🛡️ Proposed fix
     if (i2c_controller_read(
             s_i2c,
             AHT10_ADDR,
             data,
             sizeof(data),
             pdMS_TO_TICKS(100)) != ERROR_NONE)
         return false;

+    // Verify sensor is not busy and is calibrated
+    if ((data[0] & 0x80) || !(data[0] & 0x08))
+        return false;
+
     uint32_t raw_h =
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uint8_t data[6];
if (i2c_controller_read(
s_i2c,
AHT10_ADDR,
data,
sizeof(data),
pdMS_TO_TICKS(100)) != ERROR_NONE)
return false;
uint8_t data[6];
if (i2c_controller_read(
s_i2c,
AHT10_ADDR,
data,
sizeof(data),
pdMS_TO_TICKS(100)) != ERROR_NONE)
return false;
// Verify sensor is not busy and is calibrated
if ((data[0] & 0x80) || !(data[0] & 0x08))
return false;


uint32_t raw_h =
((uint32_t)data[1] << 12) |
((uint32_t)data[2] << 4) |
(data[3] >> 4);

uint32_t raw_t =
((uint32_t)(data[3] & 0x0F) << 16) |
((uint32_t)data[4] << 8) |
data[5];

*humidity = raw_h * 100.0f / 1048576.0f;
*temperature = raw_t * 200.0f / 1048576.0f - 50.0f;

return true;
}

14 changes: 14 additions & 0 deletions Apps/AHT10Sensor/main/Source/aht10.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#pragma once
#include <stdbool.h>

#ifdef __cplusplus
extern "C" {
#endif

bool aht10_init(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

Drivers should not have an init call. Init should be part of the start/stop behaviour of the driver.

bool aht10_read(float* temperature, float* humidity);

#ifdef __cplusplus
}
#endif

103 changes: 103 additions & 0 deletions Apps/AHT10Sensor/main/Source/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
#include <tt_app.h>
#include <tt_lvgl_toolbar.h>

#include "aht10.h"

#include <lvgl.h>
#include <stdio.h>

static lv_obj_t* arc_temp;
static lv_obj_t* label_temp;
static lv_obj_t* label_hum;

static lv_timer_t* sensor_timer = NULL;

/* ------------------------------ */
/* Sensor update */
/* ------------------------------ */

static void sensor_update(lv_timer_t* timer)
{
if (!label_temp || !label_hum)
return;
Comment on lines +21 to +22
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

arc_temp missing from the null guard.

label_temp and label_hum are guarded but arc_temp is used on line 32 without a null check. While onHideApp deletes the timer before nulling the pointer (making this safe today), the guard is inconsistent and becomes a latent NPE if the cleanup order ever changes.

🛡️ Proposed fix
-    if (!label_temp || !label_hum)
+    if (!arc_temp || !label_temp || !label_hum)
         return;


float t, h;

if (!aht10_read(&t, &h))
return;

char buf[32];

int t_int = (int)t;
lv_arc_set_value(arc_temp, t_int);

snprintf(buf, sizeof(buf), "%.1f °C", t);
lv_label_set_text(label_temp, buf);

snprintf(buf, sizeof(buf), "Humidity %.1f %%", h);
lv_label_set_text(label_hum, buf);
}

/* ------------------------------ */
/* Show app */
/* ------------------------------ */

static void onShowApp(AppHandle app, void* data, lv_obj_t* parent)
{
lv_obj_t* toolbar =
tt_lvgl_toolbar_create_for_app(parent, app);
lv_obj_align(toolbar, LV_ALIGN_TOP_MID, 0, 0);

arc_temp = lv_arc_create(parent);
lv_obj_set_size(arc_temp, 200, 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to fit on small displays.

lv_obj_align(arc_temp, LV_ALIGN_CENTER, 0, 30);

lv_arc_set_range(arc_temp, 0, 50);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Arc range 0–50°C is too narrow for the AHT10's full output range.

The AHT10 measures from −40°C to +85°C. Temperatures outside 0–50°C will be silently clamped by lv_arc_set_value, showing incorrect arc position while the label displays the true value — creating a confusing visual mismatch.

lv_arc_set_value(arc_temp, 25);

lv_arc_set_rotation(arc_temp, 135);
lv_arc_set_bg_angles(arc_temp, 0, 270);

label_temp = lv_label_create(parent);
lv_label_set_text(label_temp, "--.- °C");
lv_obj_align(label_temp, LV_ALIGN_CENTER, 0, 10);

label_hum = lv_label_create(parent);
lv_label_set_text(label_hum, "Humidity --%");
lv_obj_align(label_hum, LV_ALIGN_CENTER, 0, 60);

aht10_init();
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

aht10_init() return value silently discarded.

If the sensor is absent or I2C setup fails, the app proceeds normally — the timer fires, aht10_read fails immediately, and the UI remains stuck on placeholder values with no indication of failure. At minimum, consider logging the error or displaying an error state in the UI.


sensor_timer = lv_timer_create(sensor_update, 2000, NULL);
}

/* ------------------------------ */
/* Hide app (IMPORTANT FIX) */
/* ------------------------------ */

static void onHideApp(AppHandle app, void* data)
{
if (sensor_timer) {
lv_timer_del(sensor_timer);
sensor_timer = NULL;
}

label_temp = NULL;
label_hum = NULL;
arc_temp = NULL;
}

/* ------------------------------ */
/* Entry */
/* ------------------------------ */

int main(int argc, char* argv[])
{
tt_app_register((AppRegistration) {
.onShow = onShowApp,
.onHide = onHideApp
});

return 0;
}

10 changes: 10 additions & 0 deletions Apps/AHT10Sensor/manifest.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[manifest]
version=0.1
[target]
sdk=0.7.0-dev
platforms=esp32
[app]
id=one.tactility.aht10
versionName=0.1.0
versionCode=1
name=AHT10 Sensor
1 change: 1 addition & 0 deletions Apps/AHT10Sensor/tactility.py