Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement end-to-end WiFI functionality #127

Draft
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

jmadden173
Copy link
Contributor

@jmadden173 jmadden173 commented Aug 23, 2024

Name/Affiliation/Title
John, Maintainer

Purpose of the PR
Add end-to-end support for uploading via WiFi. Starting at the stm32, a command should be sent to the esp32 which uploads a measurement to Dirtviz.

Development Environment
Provide the OS, hardware version, Platformio version, and information about any other tools used during the development.

Test Procedure
TBD

Additional Context
Incremental PR for buffer support. This PR adds the ModuleHandler class that handles directing arbitrary command to various "modules" over the i2c interface between the stm32 and esp32. Based on the message type, the data gets "forwarded" to the correct module for processing. The WiFi module is the first implemented one. The commands are encoded via protobuf that tell the esp32 to connect and return the current timestamp as RTC synchronization and an upload command for actual data.

Task List

  • Update CHANGELOG.md
  • Static code analysis passes
  • All environments can be built
  • All tests pass
  • Clear documentation for new code
  • Linting passes
  • (If applicable) Version bump python library

Relevant Issues
Closes #46

@jmadden173 jmadden173 self-assigned this Aug 23, 2024
@jmadden173 jmadden173 linked an issue Aug 23, 2024 that may be closed by this pull request
@jmadden173
Copy link
Contributor Author

When merging closed duplicate/related PRs: #76 #89

@gemarcano
Copy link

I'm looking through the code still, but some things that stand out from a style perspective:

  • C++ doesn't need void when the parameter list in functions is empty
  • C++ doesn't need this-> for accessing member variables
  • Almost no need to use typedefs in C++, especially for structs and enums.
  • Because this is C++11, you can probably use std::array instead of bare arrays. std::array doesn't require any allocator or anything, it's pretty much just a bare array in a trench coat, with some supporting functions that are nice (can even use std::array in for loops!)

Copy link

@gemarcano gemarcano left a comment

Choose a reason for hiding this comment

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

I've done a first pass review. I didn't focus too much on the python code, mostly on the C++ and C code.

The main class of bugs I found are handling arrays and their sizes. Some APIs need to be tweaked to ether take in an std::array with a known fixed size, or allow passing in both a pointer and a size of the array, and actually check it internally.

Additionally, I don't think malloc is needed in most cases it's being used, unless I'm mising something.

std::map<int, Module*> req_map;

/** Store reference to last module */
Module* last_module;

Choose a reason for hiding this comment

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

I'll check once I get to the constructor, but this really needs to be always defined as something (even if just nullptr). Before access it should always be checked as well.


typedef struct {
/** Size of receive/request buffer */
static const size_t size = Esp32Command_size;

Choose a reason for hiding this comment

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

Why have a constant in a struct definition? Might as well make it a global constant (looks like Esp32Command_size is already one).

/** Size of receive/request buffer */
static const size_t size = Esp32Command_size;
/** Buffer to store data */
uint8_t data[size] = {};

Choose a reason for hiding this comment

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

Might want to use std::array instead of raw arrays.

#ifdef UNIT_TEST
static const uint8_t module_handler_buffer_size = 32;
static uint8_t module_handler_rx_buffer[module_handler_buffer_size] = {};
static uint8_t module_handler_tx_buffer[module_handler_buffer_size] = {};

Choose a reason for hiding this comment

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

Could use std::array.

/** Store reference to last module */
Module* last_module;

typedef struct {

Choose a reason for hiding this comment

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

This is C++, no need to use typedef here.

} else {
// create small buffer
Buffer chunk = {};
chunk.data = malloc(g_i2c_buffer_size);

Choose a reason for hiding this comment

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

Same here. Is dynamic memory needed?

} Buffer;

/** @brief Buffer for ControllerTransmit */
static Buffer tx = {};

Choose a reason for hiding this comment

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

This construct makes a Buffer variable per translation unit (i.e. file) that includes this header. Is this intentional? Or is the intent to have global tx and rx buffers?

Additionally, in C I think = {0} is needed to zero initialize stuff.

tx.len = 0;

// allocate rx buffer
rx.data = (uint8_t*) malloc(buffer_size);

Choose a reason for hiding this comment

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

Is dynamic memory needed here? Esp32Command_size is known beforehand (right?)

cmd = DecodeEsp32Command(rx.data, rx.len);

// copy response
memcpy(resp, cmd.command.wifi_command.resp.bytes, cmd.command.wifi_command.resp.size);

Choose a reason for hiding this comment

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

a check to see if it's safe to copy cmd.command.wifi_command.resp.size into resp needs to be done.

Choose a reason for hiding this comment

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

Where is this data structure used? It's introduced in this pull request, but I don't see anything actually using it other than tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support extending buffer size with sd card Add WiFi functionality through the ESP32 (Due: 2024-03-15)
2 participants