Repository: ClemensElflein/OpenMower
File: Firmware/LowLevel/src/main.cpp (C++)

Code before:

void sendConfigMessage(const uint8_t pkt_type) {
    size_t msg_size = sizeof(struct ll_high_level_config) + 3;  // + 1 type + 2 crc
    uint8_t *msg = (uint8_t *)malloc(msg_size);
    if (msg == NULL)
        return;
    *msg = pkt_type;
    memcpy(msg + 1, &llhl_config, sizeof(struct ll_high_level_config));  // Copy our live config into the message, behind type
    sendMessage(msg, msg_size);
    free(msg);
}

void applyConfig(const uint8_t *buffer, const size_t size) {
    // This is a flexible length packet where the size may vary when ll_high_level_config struct got enhanced only on one side.
    // If payload size is larger than our struct size, ensure that we only copy those we know of = our struct size.
    // If payload size is smaller than our struct size, copy only the payload we got, but ensure that the unsent member(s) have reasonable defaults.
    size_t payload_size = min(sizeof(ll_high_level_config), size - 2);  // exclude crc

    // Use a temporary config for easier sanity adaption and copy our live config, which has at least reasonable defaults.
    // The live config copy ensures that we've reasonable values for the case that HL config struct is older (smaller) than ours.
    auto hl_config = llhl_config;

    // Copy payload to temporary config
    memcpy(&hl_config, buffer, payload_size);

    // Takeover of HL leading config values
    llhl_config.config_bitmask = (llhl_config.config_bitmask & ~LL_HIGH_LEVEL_CONFIG_BIT_HL_IS_LEADING) | (hl_config.config_bitmask & LL_HIGH_LEVEL_CONFIG_BIT_HL_IS_LEADING);

    // Set HL leading member if they're not "undefined"
    // TODO: Not that nice :-/ Is there no more clever way? Even with a templated function, I do see similar 8 lines here

    if (hl_config.rain_threshold != 0xffff) llhl_config.rain_threshold = hl_config.rain_threshold;
    if (!std::isnan(hl_config.v_charge_cutoff)) llhl_config.v_charge_cutoff = min(hl_config.v_charge_cutoff, 40.0f);  // Absolute max. limited by D2/D3 Schottky
    if (!std::isnan(hl_config.i_charge_cutoff)) llhl_config.i_charge_cutoff = min(hl_config.i_charge_cutoff, 5.0f);   // Absolute max. limited by D2/D3 Schottky
    if (!std::isnan(hl_config.v_battery_cutoff)) llhl_config.v_battery_cutoff = hl_config.v_battery_cutoff;
    if (!std::isnan(hl_config.v_battery_empty)) llhl_config.v_battery_empty = hl_config.v_battery_empty;
    if (!std::isnan(hl_config.v_battery_full)) llhl_config.v_battery_full = hl_config.v_battery_full;
    if (hl_config.lift_period != 0xffff) hl_config.lift_period = hl_config.lift_period;
    if (hl_config.tilt_period != 0xffff) hl_config.tilt_period = hl_config.tilt_period;

    // HL is always leading for language
    strncpy(llhl_config.language, hl_config.language, 2);

    // HL send 0xff if it doesn't provide a volume
    if (hl_config.volume != 0xff) llhl_config.volume = llhl_config.volume;

    // Take over those hall inputs which are not undefined
    for (size_t i = 0; i < MAX_HALL_INPUTS; i++) {
        if (hl_config.hall_configs[i].mode != HallMode::UNDEFINED)
            llhl_config.hall_configs[i] = hl_config.hall_configs[i];
    }

Review comment (performance):
Mind explaining what all this logic for "HL leading" is about? I thought that HL would always send the full config and we take over everything (expect on version mismatch, but I think we can ignore that for a second). You rather want to be able to send a delta config, which keeps "undefined" fields as they are? Is that really necessary and worth the complexity? I think you mentioned volume which can be set on the mower, but is there anything else that would be controlled from LL?

Code after:
        struct msg_event_rain *msg = (struct msg_event_rain *)buffer;
        stock_ui_rain = (msg->value < llhl_config.rain_threshold);
    }
    else if (buffer[0] == Get_Subscribe && size == sizeof(struct msg_event_subscribe))
    {
        struct msg_event_subscribe *msg = (struct msg_event_subscribe *)buffer;
        ui_topic_bitmask = msg->topic_bitmask;
        ui_interval = msg->interval;
    }
}

void sendConfigMessage(const uint8_t pkt_type) {
    size_t msg_size = sizeof(struct ll_high_level_config) + 3;  // + 1 type + 2 crc
    uint8_t *msg = (uint8_t *)malloc(msg_size);
    if (msg == NULL)
        return;
    msg[0] = pkt_type;
    memcpy(msg + 1, &llhl_config, sizeof(struct ll_high_level_config));  // Copy our live config into the message, behind type
    sendMessage(msg, msg_size);                                          // sendMessage() also calculate the packet CRC
    free(msg);
}

/**
 * @brief applyConfig applies those members who are not undefined/unknown.
 * This function get called either when receiving a ll_high_level_config packet from HL, or during boot after read from LittleFS
 * @param buffer
 * @param size of buffer (without packet type nor CRC)
 */
void applyConfig(const uint8_t *buffer, const size_t size) {
    // This is a flexible length packet where the size may vary when ll_high_level_config struct get enhanced only on one side.
    // If payload size is larger than our struct size, ensure that we only copy those we know of (which is our struct size).
    // If payload size is smaller than our struct size, copy only the payload we got, but ensure that the unsent member(s) have reasonable defaults.
    size_t payload_size = min(sizeof(ll_high_level_config), size);

    // Use a temporary rcv_config for easier member access.
    // If payload is smaller (older), struct already contains reasonable defaults.
    ll_high_level_config rcv_config;
    memcpy(&rcv_config, buffer, payload_size);  // Copy payload to temporary config

    // Use a temporary new_config for save sanity adaption before copy to our live config.
    // It's important to use a clean one with LL-default values, and not the current live llhl_config one,
    // because we need to ensure that re-commented mower_config parameter get the LL-default value and not the last saved one!
    ll_high_level_config new_config;

    // HL always force language
    strncpy(new_config.language, rcv_config.language, 2);

    // Take over only those (HL) values which are not "undefined/unknown"
    if (rcv_config.options.dfp_is_5v != OptionState::UNDEFINED) new_config.options.dfp_is_5v = rcv_config.options.dfp_is_5v;
    if (rcv_config.options.background_sounds != OptionState::UNDEFINED) new_config.options.background_sounds = rcv_config.options.background_sounds;
    if (rcv_config.options.ignore_charging_current != OptionState::UNDEFINED) new_config.options.ignore_charging_current = rcv_config.options.ignore_charging_current;