Building Complex and Flexible System Architectures with C Language

In complex embedded systems (such as multi-device driver frameworks, protocol stacks, state machines), good code organization is often required. By using structures and function pointers, we can simulate the three main characteristics of object-oriented programming in C language: Encapsulation, Inheritance, and Polymorphism.

Implementation of Object-Oriented Concepts in C

Encapsulation

Principle: Encapsulate data and operations on that data together, hiding internal implementation details.

C Language Implementation:

  • • Use structures to store data members
  • • Define structures in <span>.c</span> files, with header files providing only forward declarations
  • • Access and manipulate data through function interfaces
// device.h - External interface
typedef struct Device Device; // Opaque pointer

Device* device_create(void);
void device_destroy(Device* dev);
int device_init(Device* dev, uint32_t config);
int device_read(Device* dev, uint8_t* buffer, size_t len);
int device_write(Device* dev, const uint8_t* data, size_t len);

// device.c - Internal implementation
struct Device {
    uint32_t id;
    uint32_t state;
    void* private_data;  // Private data
};

Device* device_create(void) {
    Device* dev = malloc(sizeof(Device));
    if (dev) {
        dev->id = 0;
        dev->state = DEVICE_STATE_INIT;
        dev->private_data = NULL;
    }
    return dev;
}

Inheritance

Principle: Subclasses inherit properties and methods from parent classes, allowing for extension or overriding.

C Language Implementation:

  • • Include the parent class structure as the first member of the subclass structure (structure layout compatibility)
  • • Subclasses can safely cast to parent class pointers
  • • Implement method overriding through function pointers
// Base class
typedef struct {
    uint32_t type;
    uint32_t state;
    int (*init)(void* self);
    int (*read)(void* self, uint8_t* buf, size_t len);
    int (*write)(void* self, const uint8_t* data, size_t len);
} BaseDevice;

// Derived class: SPI device
typedef struct {
    BaseDevice base;      // Base class as the first member
    SPI_HandleTypeDef* spi_handle;
    GPIO_TypeDef* cs_port;
    uint16_t cs_pin;
} SPIDevice;

// Derived class: I2C device
typedef struct {
    BaseDevice base;      // Base class as the first member
    I2C_HandleTypeDef* i2c_handle;
    uint8_t device_addr;
} I2CDevice;

Polymorphism

Principle: The same interface can have different implementations, with the appropriate method being called at runtime based on the object type.

C Language Implementation:

  • • Use function pointers as a “virtual function table”
  • • Each object instance contains function pointers to its methods
  • • Call through function pointers to achieve runtime polymorphism
// Base class method (virtual function)
int base_device_read(void* self, uint8_t* buf, size_t len) {
    BaseDevice* dev = (BaseDevice*)self;
    // Base class default implementation or abstract method
    return -1;  // Not implemented
}

// SPI device method implementation
int spi_device_read(void* self, uint8_t* buf, size_t len) {
    SPIDevice* spi_dev = (SPIDevice*)self;
    // SPI specific read implementation
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_RESET);
    HAL_SPI_Receive(spi_dev->spi_handle, buf, len, HAL_MAX_DELAY);
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_SET);
    return len;
}

// I2C device method implementation
int i2c_device_read(void* self, uint8_t* buf, size_t len) {
    I2CDevice* i2c_dev = (I2CDevice*)self;
    // I2C specific read implementation
    HAL_I2C_Master_Receive(i2c_dev->i2c_handle, 
                          i2c_dev->device_addr << 1, 
                          buf, len, HAL_MAX_DELAY);
    return len;
}

// Polymorphic call
int device_read(BaseDevice* dev, uint8_t* buf, size_t len) {
    return dev->read(dev, buf, len);  // Call through function pointer
}

Device Driver Framework Design

Framework Architecture Design

Design a generic device driver framework that supports multiple communication interfaces (SPI, I2C, UART) and can easily extend to new device types.

BaseDevice base class virtual function table + data members
SPIDevice SPI device
I2CDevice I2C device
UARTDevice UART device
SPI Flash specific device
SPI Sensor specific device
I2C EEPROM specific device
I2C Sensor specific device
UART GPS specific device

Class Relationship Diagram

Inheritance


Inheritance


Inheritance

BaseDevice

+DeviceType type

+DeviceState state

+uint32_t id

+int (init)(BaseDevice, void*)

+int (read)(BaseDevice, uint8_t*, size_t)

+int (write)(BaseDevice, const uint8_t*, size_t)

+int (ioctl)(BaseDevice, uint32_t, void*)

+void (destroy)(BaseDevice)

SPIDevice

+BaseDevice base

+SPI_HandleTypeDef* spi_handle

+GPIO_TypeDef* cs_port

+uint16_t cs_pin

+spi_device_read()

+spi_device_write()

I2CDevice

+BaseDevice base

+I2C_HandleTypeDef* i2c_handle

+uint8_t device_addr

+i2c_device_read()

+i2c_device_write()

UARTDevice

+BaseDevice base

+UART_HandleTypeDef* uart_handle

+uart_device_read()

+uart_device_write()

Core Data Structure Definitions

// device_driver.h
#ifndef DEVICE_DRIVER_H
#define DEVICE_DRIVER_H

#include <stdint.h>
#include <stddef.h>
#include <stdbool.h>

// Device state enumeration
typedef enum {
    DEVICE_STATE_UNINIT = 0,
    DEVICE_STATE_INIT,
    DEVICE_STATE_READY,
    DEVICE_STATE_BUSY,
    DEVICE_STATE_ERROR
} DeviceState;

// Device type enumeration
typedef enum {
    DEVICE_TYPE_SPI = 0,
    DEVICE_TYPE_I2C,
    DEVICE_TYPE_UART,
    DEVICE_TYPE_MAX
} DeviceType;

// Forward declaration
typedef struct BaseDevice BaseDevice;

// Base class structure (virtual function table + data members)
struct BaseDevice {
    // Data members
    DeviceType type;
    DeviceState state;
    uint32_t id;
    uint32_t error_code;
    
    // Virtual function table (function pointers)
    int (*init)(BaseDevice* self, void* config);
    int (*deinit)(BaseDevice* self);
    int (*read)(BaseDevice* self, uint8_t* buffer, size_t length);
    int (*write)(BaseDevice* self, const uint8_t* data, size_t length);
    int (*ioctl)(BaseDevice* self, uint32_t cmd, void* arg);
    void (*destroy)(BaseDevice* self);
    
    // Private data pointer (for storing derived class specific data)
    void* private_data;
};

// Public interface functions
BaseDevice* device_create(DeviceType type, void* config);
void device_destroy(BaseDevice* device);
int device_init(BaseDevice* device, void* config);
int device_read(BaseDevice* device, uint8_t* buffer, size_t length);
int device_write(BaseDevice* device, const uint8_t* data, size_t length);
int device_ioctl(BaseDevice* device, uint32_t cmd, void* arg);
DeviceState device_get_state(BaseDevice* device);
const char* device_get_type_string(BaseDevice* device);

#endif // DEVICE_DRIVER_H

SPI Device Implementation

// spi_device.h
#ifndef SPI_DEVICE_H
#define SPI_DEVICE_H

#include "device_driver.h"
#include "stm32f4xx_hal.h"

// SPI device configuration structure
typedef struct {
    SPI_HandleTypeDef* spi_handle;
    GPIO_TypeDef* cs_port;
    uint16_t cs_pin;
    uint32_t timeout_ms;
} SPIConfig;

// SPI device structure (inherits from BaseDevice)
typedef struct {
    BaseDevice base;           // Base class as the first member
    SPI_HandleTypeDef* spi_handle;
    GPIO_TypeDef* cs_port;
    uint16_t cs_pin;
    uint32_t timeout_ms;
} SPIDevice;

// SPI device creation function
BaseDevice* spi_device_create(SPIConfig* config);

#endif // SPI_DEVICE_H

// spi_device.c
#include "spi_device.h"
#include <stdlib.h>
#include <string.h>

// SPI device method implementation
static int spi_device_init(BaseDevice* self, void* config) {
    SPIDevice* spi_dev = (SPIDevice*)self;
    SPIConfig* cfg = (SPIConfig*)config;
    
    if (!spi_dev || !cfg) {
        return -1;
    }
    
    // Initialize SPI device specific data
    spi_dev->spi_handle = cfg->spi_handle;
    spi_dev->cs_port = cfg->cs_port;
    spi_dev->cs_pin = cfg->cs_pin;
    spi_dev->timeout_ms = cfg->timeout_ms;
    
    // Initialize CS pin
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_SET);
    
    self->state = DEVICE_STATE_READY;
    return 0;
}

static int spi_device_deinit(BaseDevice* self) {
    SPIDevice* spi_dev = (SPIDevice*)self;
    
    if (!spi_dev) {
        return -1;
    }
    
    // Release CS pin
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_SET);
    
    self->state = DEVICE_STATE_UNINIT;
    return 0;
}

static int spi_device_read(BaseDevice* self, uint8_t* buffer, size_t length) {
    SPIDevice* spi_dev = (SPIDevice*)self;
    HAL_StatusTypeDef status;
    
    if (!spi_dev || !buffer || length == 0) {
        return -1;
    }
    
    if (self->state != DEVICE_STATE_READY) {
        return -2;
    }
    
    self->state = DEVICE_STATE_BUSY;
    
    // Chip select low
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_RESET);
    
    // SPI read
    status = HAL_SPI_Receive(spi_dev->spi_handle, buffer, length, 
                             spi_dev->timeout_ms);
    
    // Chip select high
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_SET);
    
    self->state = DEVICE_STATE_READY;
    
    if (status != HAL_OK) {
        self->state = DEVICE_STATE_ERROR;
        self->error_code = status;
        return -3;
    }
    
    return length;
}

static int spi_device_write(BaseDevice* self, const uint8_t* data, size_t length) {
    SPIDevice* spi_dev = (SPIDevice*)self;
    HAL_StatusTypeDef status;
    
    if (!spi_dev || !data || length == 0) {
        return -1;
    }
    
    if (self->state != DEVICE_STATE_READY) {
        return -2;
    }
    
    self->state = DEVICE_STATE_BUSY;
    
    // Chip select low
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_RESET);
    
    // SPI write
    status = HAL_SPI_Transmit(spi_dev->spi_handle, (uint8_t*)data, length, 
                              spi_dev->timeout_ms);
    
    // Chip select high
    HAL_GPIO_WritePin(spi_dev->cs_port, spi_dev->cs_pin, GPIO_PIN_SET);
    
    self->state = DEVICE_STATE_READY;
    
    if (status != HAL_OK) {
        self->state = DEVICE_STATE_ERROR;
        self->error_code = status;
        return -3;
    }
    
    return length;
}

static int spi_device_ioctl(BaseDevice* self, uint32_t cmd, void* arg) {
    SPIDevice* spi_dev = (SPIDevice*)self;
    
    switch (cmd) {
        case SPI_IOCTL_SET_TIMEOUT:
            if (arg) {
                spi_dev->timeout_ms = *(uint32_t*)arg;
                return 0;
            }
            break;
        case SPI_IOCTL_GET_TIMEOUT:
            if (arg) {
                *(uint32_t*)arg = spi_dev->timeout_ms;
                return 0;
            }
            break;
        default:
            return -1;
    }
    return -1;
}

static void spi_device_destroy(BaseDevice* self) {
    if (self) {
        spi_device_deinit(self);
        free(self);
    }
}

// SPI device creation function (constructor)
BaseDevice* spi_device_create(SPIConfig* config) {
    SPIDevice* spi_dev = (SPIDevice*)malloc(sizeof(SPIDevice));
    
    if (!spi_dev) {
        return NULL;
    }
    
    // Initialize base class members
    memset(spi_dev, 0, sizeof(SPIDevice));
    spi_dev->base.type = DEVICE_TYPE_SPI;
    spi_dev->base.state = DEVICE_STATE_UNINIT;
    spi_dev->base.id = 0;
    spi_dev->base.error_code = 0;
    
    // Bind virtual functions (method overriding)
    spi_dev->base.init = spi_device_init;
    spi_dev->base.deinit = spi_device_deinit;
    spi_dev->base.read = spi_device_read;
    spi_dev->base.write = spi_device_write;
    spi_dev->base.ioctl = spi_device_ioctl;
    spi_dev->base.destroy = spi_device_destroy;
    
    // Initialize SPI specific data
    if (config) {
        spi_dev->spi_handle = config->spi_handle;
        spi_dev->cs_port = config->cs_port;
        spi_dev->cs_pin = config->cs_pin;
        spi_dev->timeout_ms = config->timeout_ms;
    }
    
    return (BaseDevice*)spi_dev;
}

I2C Device Implementation

// i2c_device.h
#ifndef I2C_DEVICE_H
#define I2C_DEVICE_H

#include "device_driver.h"
#include "stm32f4xx_hal.h"

// I2C device configuration structure
typedef struct {
    I2C_HandleTypeDef* i2c_handle;
    uint8_t device_addr;
    uint32_t timeout_ms;
} I2CConfig;

// I2C device structure
typedef struct {
    BaseDevice base;
    I2C_HandleTypeDef* i2c_handle;
    uint8_t device_addr;
    uint32_t timeout_ms;
} I2CDevice;

BaseDevice* i2c_device_create(I2CConfig* config);

#endif // I2C_DEVICE_H

// i2c_device.c
#include "i2c_device.h"
#include <stdlib.h>
#include <string.h>

static int i2c_device_init(BaseDevice* self, void* config) {
    I2CDevice* i2c_dev = (I2CDevice*)self;
    I2CConfig* cfg = (I2CConfig*)config;
    
    if (!i2c_dev || !cfg) {
        return -1;
    }
    
    i2c_dev->i2c_handle = cfg->i2c_handle;
    i2c_dev->device_addr = cfg->device_addr;
    i2c_dev->timeout_ms = cfg->timeout_ms;
    
    self->state = DEVICE_STATE_READY;
    return 0;
}

static int i2c_device_deinit(BaseDevice* self) {
    if (!self) {
        return -1;
    }
    
    self->state = DEVICE_STATE_UNINIT;
    return 0;
}

static int i2c_device_read(BaseDevice* self, uint8_t* buffer, size_t length) {
    I2CDevice* i2c_dev = (I2CDevice*)self;
    HAL_StatusTypeDef status;
    
    if (!i2c_dev || !buffer || length == 0) {
        return -1;
    }
    
    if (self->state != DEVICE_STATE_READY) {
        return -2;
    }
    
    self->state = DEVICE_STATE_BUSY;
    
    // I2C read
    status = HAL_I2C_Master_Receive(i2c_dev->i2c_handle,
                                    i2c_dev->device_addr << 1,
                                    buffer, length,
                                    i2c_dev->timeout_ms);
    
    self->state = DEVICE_STATE_READY;
    
    if (status != HAL_OK) {
        self->state = DEVICE_STATE_ERROR;
        self->error_code = status;
        return -3;
    }
    
    return length;
}

static int i2c_device_write(BaseDevice* self, const uint8_t* data, size_t length) {
    I2CDevice* i2c_dev = (I2CDevice*)self;
    HAL_StatusTypeDef status;
    
    if (!i2c_dev || !data || length == 0) {
        return -1;
    }
    
    if (self->state != DEVICE_STATE_READY) {
        return -2;
    }
    
    self->state = DEVICE_STATE_BUSY;
    
    // I2C write
    status = HAL_I2C_Master_Transmit(i2c_dev->i2c_handle,
                                     i2c_dev->device_addr << 1,
                                     (uint8_t*)data, length,
                                     i2c_dev->timeout_ms);
    
    self->state = DEVICE_STATE_READY;
    
    if (status != HAL_OK) {
        self->state = DEVICE_STATE_ERROR;
        self->error_code = status;
        return -3;
    }
    
    return length;
}

static int i2c_device_ioctl(BaseDevice* self, uint32_t cmd, void* arg) {
    I2CDevice* i2c_dev = (I2CDevice*)self;
    
    switch (cmd) {
        case I2C_IOCTL_SET_ADDR:
            if (arg) {
                i2c_dev->device_addr = *(uint8_t*)arg;
                return 0;
            }
            break;
        case I2C_IOCTL_GET_ADDR:
            if (arg) {
                *(uint8_t*)arg = i2c_dev->device_addr;
                return 0;
            }
            break;
        default:
            return -1;
    }
    return -1;
}

static void i2c_device_destroy(BaseDevice* self) {
    if (self) {
        i2c_device_deinit(self);
        free(self);
    }
}

BaseDevice* i2c_device_create(I2CConfig* config) {
    I2CDevice* i2c_dev = (I2CDevice*)malloc(sizeof(I2CDevice));
    
    if (!i2c_dev) {
        return NULL;
    }
    
    memset(i2c_dev, 0, sizeof(I2CDevice));
    i2c_dev->base.type = DEVICE_TYPE_I2C;
    i2c_dev->base.state = DEVICE_STATE_UNINIT;
    
    // Bind virtual functions
    i2c_dev->base.init = i2c_device_init;
    i2c_dev->base.deinit = i2c_device_deinit;
    i2c_dev->base.read = i2c_device_read;
    i2c_dev->base.write = i2c_device_write;
    i2c_dev->base.ioctl = i2c_device_ioctl;
    i2c_dev->base.destroy = i2c_device_destroy;
    
    if (config) {
        i2c_dev->i2c_handle = config->i2c_handle;
        i2c_dev->device_addr = config->device_addr;
        i2c_dev->timeout_ms = config->timeout_ms;
    }
    
    return (BaseDevice*)i2c_dev;
}

Unified Interface Implementation

// device_driver.c
#include "device_driver.h"
#include "spi_device.h"
#include "i2c_device.h"
#include <stdlib.h>

// Unified device creation interface (factory pattern)
BaseDevice* device_create(DeviceType type, void* config) {
    BaseDevice* device = NULL;
    
    switch (type) {
        case DEVICE_TYPE_SPI:
            device = spi_device_create((SPIConfig*)config);
            break;
        case DEVICE_TYPE_I2C:
            device = i2c_device_create((I2CConfig*)config);
            break;
        case DEVICE_TYPE_UART:
            // UART device implementation similar
            break;
        default:
            return NULL;
    }
    
    return device;
}

// Unified device destruction interface
void device_destroy(BaseDevice* device) {
    if (device && device->destroy) {
        device->destroy(device);
    }
}

// Unified initialization interface (polymorphic call)
int device_init(BaseDevice* device, void* config) {
    if (!device || !device->init) {
        return -1;
    }
    return device->init(device, config);
}

// Unified read interface (polymorphic call)
int device_read(BaseDevice* device, uint8_t* buffer, size_t length) {
    if (!device || !device->read) {
        return -1;
    }
    return device->read(device, buffer, length);
}

// Unified write interface (polymorphic call)
int device_write(BaseDevice* device, const uint8_t* data, size_t length) {
    if (!device || !device->write) {
        return -1;
    }
    return device->write(device, data, length);
}

// Unified control interface (polymorphic call)
int device_ioctl(BaseDevice* device, uint32_t cmd, void* arg) {
    if (!device || !device->ioctl) {
        return -1;
    }
    return device->ioctl(device, cmd, arg);
}

// Get device state
DeviceState device_get_state(BaseDevice* device) {
    if (!device) {
        return DEVICE_STATE_UNINIT;
    }
    return device->state;
}

// Get device type string
const char* device_get_type_string(BaseDevice* device) {
    if (!device) {
        return "UNKNOWN";
    }
    
    switch (device->type) {
        case DEVICE_TYPE_SPI:
            return "SPI";
        case DEVICE_TYPE_I2C:
            return "I2C";
        case DEVICE_TYPE_UART:
            return "UART";
        default:
            return "UNKNOWN";
    }
}

Application Examples

Polymorphic Call Flow

I2CDevice SPIDevice BaseDevice device_read() Application
I2CDevice SPIDevice BaseDevice device_read() Application
Create multiple devices
Unified interface call
device_create(SPI, config)
device_create(I2C, config)
device_read(spi_dev, buf, len)
Call spi_device_read() through function pointer
HAL_SPI_Receive() returns result
device_read(i2c_dev, buf, len)
Call i2c_device_read() through function pointer
HAL_I2C_Master_Receive() returns result

Extending New Device Types

Adding a new device type is very simple, just need to:

  1. 1. Define a new device structure (inherit from BaseDevice)
  2. 2. Implement all virtual functions
  3. 3. Add creation logic in the factory function
// uart_device.h
typedef struct {
    BaseDevice base;
    UART_HandleTypeDef* uart_handle;
    uint32_t timeout_ms;
} UARTDevice;

// uart_device.c
// Implement all virtual functions...
static int uart_device_read(BaseDevice* self, uint8_t* buffer, size_t length) {
    UARTDevice* uart_dev = (UARTDevice*)self;
    HAL_StatusTypeDef status;
    
    status = HAL_UART_Receive(uart_dev->uart_handle, buffer, length, 
                              uart_dev->timeout_ms);
    return (status == HAL_OK) ? length : -1;
}

// Add in device_create:
switch (type) {
    case DEVICE_TYPE_UART:
        device = uart_device_create((UARTConfig*)config);
        break;
}

Design Pattern Applications

Factory Pattern

<span>device_create()</span> function implements the factory pattern, creating corresponding device instances based on device type, hiding the specific creation details.

SPI

I2C

UART


Application
device_create factory function
Device type?
spi_device_create
i2c_device_create
uart_device_create
Returns BaseDevice*

Strategy Pattern

Different device types implement different communication strategies (SPI, I2C, UART), selecting the strategy at runtime through function pointers.

BaseDevice
read function pointer
SPI strategy spi_device_read
I2C strategy i2c_device_read
UART strategy uart_device_read
HAL_SPI_Receive
HAL_I2C_Master_Receive
HAL_UART_Receive

Template Method Pattern

The base class defines a unified interface framework, while derived classes implement specific operational details.

SPI

I2C

UART


device_read template method
Check parameters
Check state
Call virtual function dev->read
Device type
SPI implementation
I2C implementation
UART implementation
Update state
Return result

Conclusion

  1. 1. Encapsulation: Hiding implementation details through opaque pointers and interface functions
  2. 2. Inheritance: Achieving single inheritance through structure nesting
  3. 3. Polymorphism: Achieving runtime polymorphism through function pointers

This design approach has advantages in embedded systems:

  • • ✅ Maintains C language efficiency: No overhead of virtual function table lookup, memory usage is controllable
  • • ✅ Improves code maintainability: Clear hierarchical structure, easy to extend and modify
  • • ✅ Enhances code readability: Unified interface, self-documenting design
  • • ✅ Supports polymorphic programming: Same interface handles different device types

Related posts

  1. Energy-Efficient Data Collection in Wireless Sensor Networks Enabled by Drones with Matlab Code
  2. Handcrafted Circuit Boards
  3. C Language: Debian Operating System
  4. Mainstream Embedded Chip Manufacturers, Representative Products, and Features
  5. Four-Year Learning Path for Embedded Engineers in University
  6. Navigating Non-Compete Agreements for Professionals Over 40
  7. Launch of the Insta360 GO Ultra Sports Camera Powered by Allwinner F133
  8. Smart Home and AIoT – In-Depth Data on Main Products: Topband Co., Ltd., Heertai, Roborock, Ecovacs, XGIMI, and EZVIZ
  9. Where Are the Most Outstanding PLC Professionals Now?
  10. The Misunderstanding: Assembly Language is Faster than C Language
  11. Overview of Semiconductor Manufacturers’ Medical Device Solutions
  12. VAN RYSEL 500 MIPS Helmet Review: High Cost-Performance
  13. Understanding MPIDR_EL1 Register Affinity in ARMv8/ARMv9
  14. Arm Ethos-U85 NPU: Implementing Generative AI on the Edge with Small Language Models
  15. Analysis of Cylinder Oil Injection Failure in Marine Main Engines
  16. Evaluation of Patriot SMI70 PCI-E 4.0 SSD with Phison SM2268XT Controller and Yangtze Memory TLC
  17. What Are the Differences Between Embedded Programming and PC Programming?
  18. In-Depth Analysis of the Exclusive Mechanism in Arm v8/v9
  19. Next-Generation AI Agents: The Integration of Event-Driven Architecture (EDA) and AI Agents

Leave a Comment