From 82d7972728523c2ade5efb46d00fb37873f6a58b Mon Sep 17 00:00:00 2001 From: joran2738 <101818067+joran2738@users.noreply.github.com> Date: Fri, 24 Nov 2023 22:38:04 +0100 Subject: [PATCH] solving reviews pt3 --- project/Core/Inc/UDP_broadcast.h | 34 ++++------- project/Core/Src/UDP_broadcast.c | 102 +++++++++++++++---------------- project/Core/Src/main.c | 12 +++- 3 files changed, 72 insertions(+), 76 deletions(-) diff --git a/project/Core/Inc/UDP_broadcast.h b/project/Core/Inc/UDP_broadcast.h index 8c9e095..a5f0047 100644 --- a/project/Core/Inc/UDP_broadcast.h +++ b/project/Core/Inc/UDP_broadcast.h @@ -19,29 +19,17 @@ #include "lcd_api.h" -// Defines used by owner details -#define SOD_NAME "set_owner_details_name" -#define GOD_NAME "get_owner_details_name" -#define SOD_SURNAME "set_owner_details_surname" -#define GOD_SURNAME "get_owner_details_surname" -#define SOD_REPLY "set_owner_details_reply" -#define GOD_REPLY "get_owner_details_reply" -#define SOD_MAC "set_owner_details_mac" -#define SOD "set_owner_details" -#define F_REPLY "format_reply" - // Defines used by UDP callback -#define MAX_DATA_SIZE 63 // Define the maximum expected data size -#define UDP_QUESTION1 "Where are you?v1.0" // Expected question from UDP client -#define FUNC_LEN 7 -#define MAX_COLON_COMMA_COUNT 4 +#define UDP_BROADCAST_MAX_DATA_SIZE 63 // Define the maximum expected data size +#define UDP_BROADCAST_UDP_QUESTION1 "Where are you?v1.0" // Expected question from UDP client +#define UDP_BROADCAST_FUNC_LEN 7 +#define UDP_BROADCAST_MAX_COLON_COMMA_COUNT 4 // Defines used by owner details - -#define MAX_NAME_SIZE 20 -#define MAX_REPLY_SIZE 120 -#define MAX_MAC_ADDR_LEN 18 -#define MAX_EXTRA_REPLY_CHARS 27 +#define UDP_BROADCAST_MAX_NAME_SIZE 21 // Code automatically leaves 1 char for '\0' (actual length = length - 1) +#define UDP_BROADCAST_MAX_REPLY_SIZE 120 // Code automatically leaves 1 char for '\0' (actual length = length - 1) +#define UDP_BROADCAST_MAX_MAC_ADDR_LEN 19 +#define UDP_BROADCAST_MAX_EXTRA_REPLY_CHARS 27 /** @@ -51,10 +39,10 @@ */ typedef struct { - char name[MAX_NAME_SIZE]; - char surname[MAX_NAME_SIZE]; + char name[UDP_BROADCAST_MAX_NAME_SIZE]; + char surname[UDP_BROADCAST_MAX_NAME_SIZE]; uint8_t mac_address[6]; - char reply[MAX_REPLY_SIZE]; + char reply[UDP_BROADCAST_MAX_REPLY_SIZE]; } owner_details_t; // The following functions are used for owner details (those that must be available in main) diff --git a/project/Core/Src/UDP_broadcast.c b/project/Core/Src/UDP_broadcast.c index c537338..ae823f5 100644 --- a/project/Core/Src/UDP_broadcast.c +++ b/project/Core/Src/UDP_broadcast.c @@ -50,20 +50,20 @@ static void udp_broadcast_set_owner_details_mac(void) { * * @param[in] name string containing the owner's name * @return setting owner name error - * - 1: no error occured, name was set - * - 0: an error occured, name pointer is NULL + * - 0: no error occured, name was set + * - 1: an error occured, name pointer is NULL */ static uint8_t udp_broadcast_set_owner_details_name(const char* name) { if (name == NULL) { - LOG_WARN(TAG, "%s: string given is a NULL pointer", SOD_NAME); - return 0; + LOG_WARN(TAG, "set_owner_details_name: string given is a NULL pointer"); + return 1; } LOG_DEBUG(TAG, "set: %s", name); lcd_display_text(" ", owner_name_x_pos, owner_name_y_pos, LCD_GREEN,LCD_FONT16); - strncpy(udp_owner.name, name, sizeof(udp_owner.name)); + strncpy(udp_owner.name, name, sizeof(udp_owner.name)-1); // -1: compensate for '\0' lcd_display_text(name, owner_name_x_pos, owner_name_y_pos, LCD_GREEN, LCD_FONT16); - return 1; + return 0; } /** @@ -73,17 +73,17 @@ static uint8_t udp_broadcast_set_owner_details_name(const char* name) { * * @param[in] surname string containing the owner's surname * @return setting owner surname error - * - 1: no error occured, surname was set - * - 0: an error occured, surname pointer is NULL + * - 0: no error occured, surname was set + * - 1: an error occured, surname pointer is NULL */ static uint8_t udp_broadcast_set_owner_details_surname(const char* surname) { if (surname == NULL) { - LOG_WARN(TAG, "%s: string given is a NULL pointer", SOD_SURNAME); - return 0; + LOG_WARN(TAG, "set_owner_details_surname: string given is a NULL pointer"); + return 1; } LOG_DEBUG(TAG, "set: %s", surname); - strncpy(udp_owner.surname, surname, sizeof(udp_owner.surname)); - return 1; + strncpy(udp_owner.surname, surname, sizeof(udp_owner.surname)-1); // -1: compensate for '\0' + return 0; } /** @@ -93,18 +93,18 @@ static uint8_t udp_broadcast_set_owner_details_surname(const char* surname) { * * @param[in] reply string used to reply to the UDP broadcast * @return setting owner reply error - * - 1: no error occured, reply was set - * - 0: an error occured, reply pointer is null + * - 0: no error occured, reply was set + * - 1: an error occured, reply pointer is null */ static uint8_t udp_broadcast_set_owner_details_reply(const char* reply) { if (reply == NULL) { - LOG_WARN(TAG, "%s: string given is a NULL pointer", SOD_REPLY); - return 0; + LOG_WARN(TAG, "set_owner_details_reply: string given is a NULL pointer"); + return 1; } LOG_DEBUG(TAG, "set: %s", reply); - strncpy(udp_owner.reply, reply, sizeof(udp_owner.reply)); - return 1; + strncpy(udp_owner.reply, reply, sizeof(udp_owner.reply)-1); // -1: compensate for '\0' + return 0; } /** @@ -115,17 +115,14 @@ static uint8_t udp_broadcast_set_owner_details_reply(const char* reply) { */ static void udp_broadcast_format_reply(void) { - size_t reply_len = 0; - char mac_addr_str[MAX_MAC_ADDR_LEN]; - char reply_buf[MAX_REPLY_SIZE]; - - reply_len = MAX_EXTRA_REPLY_CHARS + sizeof(mac_addr_str) + sizeof(udp_owner.surname) + sizeof(udp_owner.name); + char mac_addr_str[UDP_BROADCAST_MAX_MAC_ADDR_LEN]; + char reply_buf[UDP_BROADCAST_MAX_REPLY_SIZE]; snprintf(mac_addr_str, sizeof(mac_addr_str), "%02X:%02X:%02X:%02X:%02X:%02X", udp_owner.mac_address[0], udp_owner.mac_address[1], udp_owner.mac_address[2], udp_owner.mac_address[3], udp_owner.mac_address[4], udp_owner.mac_address[5]); - snprintf(reply_buf, reply_len, "%s is present and my owner is %s %s", mac_addr_str, udp_owner.surname, + snprintf(reply_buf, UDP_BROADCAST_MAX_REPLY_SIZE, "%s is present and my owner is %s %s", mac_addr_str, udp_owner.surname, udp_owner.name); udp_broadcast_set_owner_details_reply(reply_buf); } @@ -143,12 +140,14 @@ static void udp_broadcast_format_reply(void) { * - ERR_ARG. one or both arguments are NULL pointers */ err_t udp_broadcast_set_owner_details(const char* name, const char* surname) { - if (udp_broadcast_set_owner_details_name(name) && udp_broadcast_set_owner_details_surname(surname)) { + if (!udp_broadcast_set_owner_details_name(name) && !udp_broadcast_set_owner_details_surname(surname)) { + + // If both return 0 it's okay udp_broadcast_set_owner_details_mac(); udp_broadcast_format_reply(); - return ERR_ARG; + return ERR_OK; } - return ERR_OK; + return ERR_ARG; } /** @@ -195,36 +194,37 @@ char* udp_broadcast_get_owner_details_reply(void) { * @param[in] data the datagram received on port 64000 */ -static void udp_broadcast_check_function(const char data[MAX_DATA_SIZE]) { - char func[FUNC_LEN]; - char buffer[MAX_NAME_SIZE]; - uint8_t enders[MAX_COLON_COMMA_COUNT]; +static void udp_broadcast_check_function(const char data[UDP_BROADCAST_MAX_DATA_SIZE]) { + char func[UDP_BROADCAST_FUNC_LEN]; + char buffer[UDP_BROADCAST_MAX_NAME_SIZE]; + uint8_t enders[UDP_BROADCAST_MAX_COLON_COMMA_COUNT]; uint8_t counter = 0; - size_t data_len = strlen(data); + uint8_t data_len = strlen(data); memset(func, 0, sizeof(func)); memset(buffer, 0, sizeof(buffer)); - memcpy(func,data,FUNC_LEN-1); + memcpy(func,data,UDP_BROADCAST_FUNC_LEN-1); if (strcmp(func, "func1:") != 0) { LOG_WARN(TAG, "udp_broadcast_check_function: datagram does not contain function that's currently available"); return; } - for (uint8_t i = 0; i < data_len && counter < MAX_COLON_COMMA_COUNT; i++) { + for (uint8_t i = 0; i < data_len && counter < UDP_BROADCAST_MAX_COLON_COMMA_COUNT; i++) { if ((data[i] == ',' || data[i] == ':')) { enders[counter] = i; counter++; } } - if (enders[2] - enders[1] < MAX_NAME_SIZE + 2 && data_len - enders[3] < MAX_NAME_SIZE + 2 && strncmp(data + enders[0], ":name", 5) == 0 - && strncmp(data + enders[2], ", surname", 9) == 0) { + if (enders[2] - enders[1] < UDP_BROADCAST_MAX_NAME_SIZE + 1 && data_len - enders[3] < UDP_BROADCAST_MAX_NAME_SIZE + 1 + && strncmp(data + enders[0], ":name", 5) == 0 && strncmp(data + enders[2], ", surname", 9) == 0) { + counter = 0; - for (uint8_t i = enders[1] + 2; i < enders[2] && data[i] != '\0'; i++) { + for (uint8_t i = enders[1] + 2; i < enders[2]; i++) { buffer[counter] = data[i]; counter++; } - if (strcmp(buffer, "") == 0) { - strncpy(buffer, "name", sizeof(buffer)); + if (buffer[0]=='\0') { + strncpy(buffer, "name", sizeof(buffer)-1); // -1: compensate for '\0', just for safety, not really needed } LOG_INFO(TAG, "new owner name:%s", buffer); udp_broadcast_set_owner_details_name(buffer); @@ -235,7 +235,7 @@ static void udp_broadcast_check_function(const char data[MAX_DATA_SIZE]) { counter++; } if (buffer[0]=='\0') { - strncpy(buffer, "default", sizeof(buffer)); + strncpy(buffer, "default", sizeof(buffer)-1); // -1: compensate for '\0', just for safety, not really needed } LOG_INFO(TAG, "new owner surname:%s", buffer); udp_broadcast_set_owner_details_surname(buffer); @@ -265,7 +265,7 @@ static void udp_receive_callback(void* arg, struct pbuf* p_data; size_t len; char* pc; - char data[MAX_DATA_SIZE]; + char data[UDP_BROADCAST_MAX_DATA_SIZE]; char source_ip_str[16]; memset(data, 0, sizeof(data)); @@ -282,8 +282,8 @@ static void udp_receive_callback(void* arg, if (p_data == NULL) { LOG_WARN(TAG, "udp_receive_callback: unable to allocate data buffer for reply"); goto defer; - } if (len > MAX_DATA_SIZE) { - LOG_WARN(TAG, "udp_receive_callback: input buffer was bigger than max size %d", MAX_DATA_SIZE); + } if (len > UDP_BROADCAST_MAX_DATA_SIZE) { + LOG_WARN(TAG, "udp_receive_callback: input buffer was bigger than max size %d", UDP_BROADCAST_MAX_DATA_SIZE); goto defer; } for (size_t i = 0; i < len; i++) { @@ -291,17 +291,17 @@ static void udp_receive_callback(void* arg, } LOG_INFO(TAG, "udp_receive_callback: received data from %s at port: %d: %s", source_ip_str, port, data); - if (strcmp(data, UDP_QUESTION1) == 0) { + if (strcmp(data, UDP_BROADCAST_UDP_QUESTION1) == 0) { p_data->payload = udp_owner.reply; p_data->len = strlen(udp_owner.reply); p_data->tot_len = strlen(udp_owner.reply); udp_sendto(connection, p_data, addr, 64000); /* Was using the sending port of the pc, * this is not the port that Qt is listening to */ - LOG_INFO(TAG, "tried to reply to %s at port: %d: %s", source_ip_str, 64000, udp_owner.reply); + LOG_INFO(TAG, "udp_receive_callback: tried to reply to %s at port: %d: %s", source_ip_str, 64000, udp_owner.reply); goto defer; } - LOG_INFO(TAG, "other function called"); + LOG_INFO(TAG, "udp_receive_callback: checking which function was called"); udp_broadcast_check_function(data); p_data->payload = udp_owner.reply; p_data->len = strlen(udp_owner.reply); @@ -309,7 +309,7 @@ static void udp_receive_callback(void* arg, udp_sendto(connection, p_data, addr, 64000); /* Was using the sending port of the pc, * this is not the port that Qt is listening to */ - LOG_INFO(TAG, "tried to reply to %s at port: %d: %s", source_ip_str, 64000, udp_owner.reply); + LOG_INFO(TAG, "udp_receive_callback: tried to reply to %s at port: %d: %s", source_ip_str, 64000, udp_owner.reply); defer: pbuf_free(p); @@ -332,20 +332,20 @@ err_t udp_broadcast_connection_init(void) { struct udp_pcb* connection; err_t err; - LOG_INFO(TAG, "initialising UDP server"); + LOG_INFO(TAG, "udp_broadcast_connection_init: initializing UDP server"); connection = udp_new(); if (connection == NULL) { - LOG_WARN(TAG, "Initializing UDP server failed, connection is null"); + LOG_WARN(TAG, "udp_broadcast_connection_init: Initializing UDP server failed, connection is null"); return ERR_MEM; } err = udp_bind(connection, IP_ANY_TYPE, 64000); if (err != ERR_OK) { - LOG_WARN(TAG, "Initializing UDP server failed, err not ok"); + LOG_WARN(TAG, "udp_broadcast_connection_init: Initializing UDP server failed, err not ok"); udp_remove(connection); return err; } udp_recv(connection, udp_receive_callback, NULL); - LOG_INFO(TAG, "Initializing UDP server successful, callback running"); + LOG_INFO(TAG, "udp_broadcast_connection_init: Initializing UDP server successful, callback running"); return err; } diff --git a/project/Core/Src/main.c b/project/Core/Src/main.c index 6506a54..53afcb7 100644 --- a/project/Core/Src/main.c +++ b/project/Core/Src/main.c @@ -135,9 +135,17 @@ int main(void) tftp_server_init(); // Initialize the UDP broadcast service - if (udp_broadcast_init(270,255) != ERR_OK || udp_broadcast_connection_init() != ERR_OK){ - LOG_WARN(TAG,"error initializing udp connection"); + + if (udp_broadcast_init(270,255) == ERR_OK){ + goto connected; } + LOG_WARN(TAG,"error initializing udp connection, trying again in 500ms"); + HAL_Delay(500); + if(udp_broadcast_connection_init() != ERR_OK){ + LOG_WARN(TAG,"error initializing udp connection, check warnings from udp_broadcast_init() or udp_broadcast_connection_init()"); + } + +connected: if (udp_broadcast_set_owner_details("Joran", "Van Nieuwenhoven")){ LOG_WARN(TAG,"error setting owner's details"); }