Feedback on SD Card Logging #305
Conversation
| import traceback | ||
| from collections import OrderedDict | ||
|
|
||
| import adafruit_pathlib |
There was a problem hiding this comment.
I previously suggested using adafruit_pathlib and you've done a good job implementing it! I spent some time with the implementation using pathlib and decided that it would cause more pain than it's worth:
- All CircuitPython functions that interact with the filesystem expect a string path which would mean that we would always need to output the path to a string anyways.
adafruit_pathlibdid not have anyjoin()semantics which made adding to aPathobject clunky. (this could be submitted upstream)- Most modules in pysquared use the
Loggerwhich means we would have needed to mock away the use ofadafruit_pathlibin many many test files.
So ultimately, I removed it.
There was a problem hiding this comment.
I spent some time on this file to reduce its complexity and to make sure the name made sense 🤞... naming is hard.
| self.colorized: bool = colorized | ||
|
|
||
| try: | ||
| self.sd_path = self.sd_path / "logs" |
There was a problem hiding this comment.
I didn't want to have the logger be opinionated about the directory location so now we just accept whatever path is provided to the logger.
pysquared/logger.py
Outdated
| if log_dir is not None: | ||
| try: | ||
| # Octal number 0o040000 is the stat mode indicating the file being stat'd is a directory | ||
| directory_mode: int = 0o040000 | ||
| st_mode = os.stat(log_dir)[0] | ||
| if st_mode != directory_mode: | ||
| raise ValueError( | ||
| f"Logging path must be a directory, received {st_mode}." | ||
| ) | ||
| except OSError as e: | ||
| raise ValueError("Invalid logging path.") from e |
There was a problem hiding this comment.
If someone sets a log location that doesn't exist or is not a directory, we'll error out.
| try: | ||
| from mocks.circuitpython.microcontroller import Processor | ||
| except ImportError: | ||
| from microcontroller import Processor | ||
| except ImportError: | ||
| from mocks.circuitpython.microcontroller import Processor |
There was a problem hiding this comment.
Not related to this PR: switching this order is possible now because of #304. With this order change, the IDE shows more accurate hints.
| try: | ||
| json_output = json.dumps(json_order) | ||
| except TypeError as e: | ||
| json_output = json.dumps( | ||
| OrderedDict( | ||
| [ | ||
| ("time", asctime), | ||
| ("level", "ERROR"), | ||
| ("msg", f"Failed to serialize log message: {e}"), | ||
| ] | ||
| ), | ||
| ) |
There was a problem hiding this comment.
This became no longer necessary after #228 but was left in. It has no tests and is no longer required so I removed it.
| "pysquared.boot", | ||
| "pysquared.config", | ||
| "pysquared.hardware", | ||
| "pysquared.hardware.imu", | ||
| "pysquared.hardware.burnwire.manager", | ||
| "pysquared.hardware.burnwire", | ||
| "pysquared.hardware.imu.manager", | ||
| "pysquared.hardware.magnetometer", | ||
| "pysquared.hardware.imu", | ||
| "pysquared.hardware.light_sensor.manager", | ||
| "pysquared.hardware.magnetometer.manager", | ||
| "pysquared.hardware.radio", | ||
| "pysquared.hardware.magnetometer", | ||
| "pysquared.hardware.power_monitor.manager", | ||
| "pysquared.hardware.power_monitor", | ||
| "pysquared.hardware.radio.manager", | ||
| "pysquared.hardware.radio.packetizer", | ||
| "pysquared.hardware.power_monitor", | ||
| "pysquared.hardware.power_monitor.manager", | ||
| "pysquared.hardware.temperature_sensor", | ||
| "pysquared.hardware.radio", | ||
| "pysquared.hardware.sd_card", | ||
| "pysquared.hardware.temperature_sensor.manager", | ||
| "pysquared.hardware.burnwire", | ||
| "pysquared.hardware.burnwire.manager", | ||
| "pysquared.hardware.light_sensor.manager", | ||
| "pysquared.hardware.temperature_sensor", | ||
| "pysquared.hardware", | ||
| "pysquared.nvm", | ||
| "pysquared.protos", | ||
| "pysquared.rtc", | ||
| "pysquared.rtc.manager", | ||
| "pysquared.rtc", |
|
asiemsen
left a comment
asiemsen
left a comment
There was a problem hiding this comment.
Looks great! I would love to go over and review the design behind the tests sometime so that I can start implementing some of these in the future! I also saw your most recent commit that uses a setter for the log directory, which I think was implemented really well. Thanks again for all your help on this.
Summary
This PR provides feedback on #299
How was this tested
boot_out.txtfirst run:Adafruit CircuitPython 9.2.8 on 2025-05-28; PROVES Kit v4 with rp2040 Board ID:proveskit_rp2040_v4 UID:E4639C3563112E2D boot.py output: Disabled USB drive Remounted root filesystem Mount point /poke created. Enabled USB driveboot_out.txtsubsequent runs:Adafruit CircuitPython 9.2.8 on 2025-05-28; PROVES Kit v4 with rp2040 Board ID:proveskit_rp2040_v4 UID:E4639C3563112E2D boot.py output: Disabled USB drive Remounted root filesystem Mount point /poke already exists. Enabled USB driveDemonstrating logging to file:
