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

Review PR: Add "Offline" SDLogger Mode #676

Open
wants to merge 211 commits into
base: main
Choose a base branch
from
Open

Conversation

brentru
Copy link
Member

@brentru brentru commented Dec 27, 2024

This pull request is not meant to be merged in. It is a review-only pull request for specific regions of code.

Codebase regions for review

  • Added a new class, ws_sdcard - for interfacing with a micro SD Card breakout.
    • Request: please take a careful look at how parseConfigFile functions and looks
    • Request: please take a careful look at how we switch between production and debug-mode in this class
    • BuildJSONDoc and LogJSONDoc API interfaces are present for each type of component
    • This class also supports a Real-Time-Clock breakout.
  • API calls for interacting with ws_sdcard and mode-switching (determining whether the device is online or offline) within WipperSnapper_V2 class in two functions -provision() and connect()
  • Added API calls for parsing the SD CS pins from the JSON file on the WIPPER drive within classes - FS_V2 and LittleFS_V2
  • Refactored network_interfaces/ to adapters/ to allow for a WipperSnapper device without a network interface (offline/).
    • Added new class, adapters/offline/ws_offline_pico to support a Raspberry Pi in offline mode.

Workflow enhancements for review:

  • Added a new GitHub Actions workflow file, run-tests.yml, to automate testing using Wokwi.

brentru added 30 commits August 13, 2024 14:34
Use latest build artifacts
New checkin model class added along with functions within .cpp to exec. it
src/Wippersnapper_V2.h Outdated Show resolved Hide resolved
src/Wippersnapper_V2.cpp Outdated Show resolved Hide resolved
src/Wippersnapper_V2.cpp Outdated Show resolved Hide resolved
@brentru
Copy link
Member Author

brentru commented Jan 13, 2025

@tyeth Please give this a re-review. I have changed what we discussed. I'm going to merge this branch into the api-v2 branch after you give the OK.

@brentru brentru requested a review from tyeth January 13, 2025 17:11
case wippersnapper_sensor_SensorType_SENSOR_TYPE_UNITLESS_PERCENT:
return "%";
case wippersnapper_sensor_SensorType_SENSOR_TYPE_AMBIENT_TEMPERATURE_FAHRENHEIT:
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's missing

case wippersnapper_sensor_SensorType_SENSOR_TYPE_UNSPECIFIED:
return "raw value";
case wippersnapper_sensor_SensorType_SENSOR_TYPE_ACCELEROMETER:
return "m/s/s";
Copy link
Contributor

Choose a reason for hiding this comment

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

feels like this should match gravity/linear-acceleration, but happy to not have it match if preferred

Copy link
Contributor

@tyeth tyeth left a comment

Choose a reason for hiding this comment

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

Minor changed in units method, the ambient fahrenheit specifically, maybe accelerometer too.

case wippersnapper_sensor_SensorType_SENSOR_TYPE_BOOLEAN:
return "BOOLEAN";
return "raw value";
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about leaving this blank, but having an empty field might be more frustrating for people

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.

2 participants