PowerMon/StructuredLogging support - #607
Conversation
|
|
|
github-actions seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
@ianmcorvidae and anyone else who is appropriate: How critical is it to keep python 3.8 support? I'd like to use the pandas data processing library for the 'analysis' apis/reporting/visualization (as opposed to device APIs). But that lib requires min python of 3.9. I've checked and even my raspberry pi could do that (raspbian is on 3.11 now). I would kinda rather not making a new 'meshtastic-analysis' python project. instead I'd like to have some new classes/code in meshtastic.analysis of this project. |
|
Python 3.8 is 5 years old now. I think that's fair to dump it, honestly. |
|
Yeah -- as I was saying in discord the other day, 3.8 is hitting formal end of life in October anyway, so I'm happy to drop support for it as part of this. Once we do so, I'll make sure to create one last 3.8-supporting version that'll warn users about it, just in case there's folks lingering on the old version. |
|
okay coolbeans @ianmcorvidae - I'm going to make this powermon/structured logging branch assume/require 3.8 and it can go in after that release. |
# Conflicts: # poetry.lock # pyproject.toml
…in works # Conflicts: # bin/regen-protobufs.sh
Including in the Poetry changes because it touches the same lines and I want to avoid hand merging ;-)
…ports. Initially only RAK4631 and heltec tracker are listed
(backwards/forwards compatible) with old firmware
# Conflicts: # poetry.lock # pyproject.toml
# Conflicts: # .vscode/launch.json # meshtastic/protobuf/config_pb2.py # meshtastic/protobuf/mesh_pb2.py # meshtastic/protobuf/mesh_pb2.pyi
# Conflicts: # .vscode/launch.json
|
I hear from another PR that you're out hiking so no rush in responding here 🙂, but: I was wondering if you'd be fine with me grabbing your LogRecord work from here after #627 goes in and amending both the code here and that code to send a consistent pubsub message that includes the full LogRecord so we can handle them consistently (and make the log lines available to library users, too). I don't want to introduce unnecessary conflicts for you to have to undo but given that stuff's making it into firmware it might be nice to get it in sooner/before the PowerMon stuff is finished. Thoughts? |
# Conflicts: # meshtastic/ble_interface.py # meshtastic/protobuf/mesh_pb2.py # meshtastic/protobuf/powermon_pb2.py # meshtastic/protobuf/powermon_pb2.pyi
|
Hi hi, Does this change look okay for ya'll to go in? I'd kinda like for this to to in now even though I still have a few more changes coming (just localized to the new PowerStress class). The runtime impact of powermon & powerstress is a small amount of CODE size and a tiny about of RAM. If the corresponding config variable is not set, the code change at runtime is minimal. If modules are being excluded it is completely removed from the build. I'm submitting for review now because I had to touch a fair # of files and I've got too many open WIP branches ;-). For early docs see meshtastic/firmware#4136. However after I'm finished with this feature (a weekish?) I'll move much of that text to become a new doc page and tutorial about how to do power measurements on boards. The main remaining work is not on the firmware or python client side but rather the analysis reporting chain (which in in a new meshtastic.analysis submodule in the python code). |
geeksville
changed the title
(note: the linter test for min/max is buggy so disabled)
|
Hi @ianmcorvidae Do you have any advice on how to make the code coverage tool happy with this? These new classes are mostly about hardware but I'm happy to add new meshtastic/tests that use my SimPowerMeter. Are the existing tests mocked enough that I can use a MeshInterface to talk to something and get through the config message flow / reach the later PowerStress stuff? I assume I'll need to somehow mock up the PowerStress module/agent that is running on the (sim) UUT. Can I sign up to do this 'later'? ;-) Because I kinda dread doing a bunch of mocking on PowerStress and then having to keep updating that mocking while I'm still developing/changing the feature. |
|
I'll try to give this a review later tonight or tomorrow and see if we can just get it in. Then I can do some reorganization of log stuff anyway, make sure this and BLE are consistent and the new stuff is accessible to library users without too much pain. As far as the code coverage, yeah, signing up to do it later is fine with me; our mocks and stuff are not in a terribly convenient place and this power stuff is still in flux even if we merge this now. #579 (if it can get done, since I haven't had much time to devote to planning it) would hopefully make testing in general easier as well, fingers crossed. |
ianmcorvidae
left a comment
ianmcorvidae
left a comment
There was a problem hiding this comment.
Looks generally good to me. Got a few questions, particularly about potentially passing LogRecords in full to pubsub instead of just the messages, so we can be a little more consistent with that stuff. I don't know as much about the actual power monitoring stuff, but as far as I'm aware it seems fine, and I know you're planning to keep updating/changing that, so I think we can let that happen and make any other changes we need to after it settles.
Since this will drop our 3.8 support and also since it depends on 2.3.15 protobufs, I'll make a release before merging it that's 2.3.14 and mark it as the last 3.8 release.
| "configurations": [ | ||
| { | ||
| "name": "meshtastic BLE", | ||
| "type": "python", |
There was a problem hiding this comment.
(comment at random line in file)
I know basically nothing about vscode so I'm just gonna trust that these settings make sense. If someone else who actually uses vscode wants to validate, feel free.
There was a problem hiding this comment.
yah - these are just vscode warned me that "python" is deprecated and the new recommended string is pydebug...
| - meshtastic.receive.user(packet) | ||
| - meshtastic.receive.data.portnum(packet) (where portnum is an integer or well known PortNum enum) | ||
| - meshtastic.node.updated(node = NodeInfo) - published when a node in the DB changes (appears, location changed, username changed, etc...) | ||
| - meshtastic.log.line(line) - a raw unparsed log line from the radio |
There was a problem hiding this comment.
I was mentioning before, I think I might change this, if that's okay (would do after merge, and I'd update the spot you've used it as well) -- I'd be changing it to pass the whole LogRecord as a dict, so that folks can pull out the message, level, and time/source (if those are provided) separately and do with them as they will. This will also mean I can have ble_interface output this same message and move all the log printing/colorizing stuff to one place, hopefully.
There was a problem hiding this comment.
yes - I agree, LogRecord is what should be passed (using the automatic KnownProtocol translation). IMO coloring should be done at the end and only for the stdout printer when it is going to an interactive terminal.
| def _printLogLine(line, interface): | ||
| """Print a line of log output.""" | ||
| if interface.debugOut == sys.stdout: | ||
| # this isn't quite correct (could cause false positives), but currently our formatting differs between different log representations |
There was a problem hiding this comment.
I think we can fix this by passing the whole LogRecord down to here and then looking at the log_record.DEBUG etc. as the BLE code was doing -- I'm happy to change that around but was wondering if there's a reason you didn't.
There was a problem hiding this comment.
yeah - I did this as a temporary hack kinda expecting that LogRecords are coming and the location for coloring will be moving. (I was just merging in recent changes from master and trying to keep em working)
| ROUTER_CLIENT: Config.DeviceConfig._Role.ValueType # 3 | ||
| """ | ||
| Description: Combination of both ROUTER and CLIENT. Not for mobile devices. | ||
| Deprecated in v2.3.15 because improper usage is impacting public meshes: Use ROUTER or CLIENT instead. |
There was a problem hiding this comment.
Presumably you needed the v2.3.15 protobufs for this, given that's where the powermon stuff gets merged? I suppose I'll use 2.3.14 as a final-version-that-supports-3.8, I think, before merging this, and then I can just pull in protobufs v2.3.15 outright.
There was a problem hiding this comment.
no idea - that string came from meshtastic/protobufs and I just merged it with my changes.
| ) | ||
|
|
||
| # We need a closure here because the subscription API is very strict about exact arg matching | ||
| def listen_glue(line, interface): # pylint: disable=unused-argument |
There was a problem hiding this comment.
(mostly noting to myself -- another spot that'll need updating in order to pass a LogRecord instead of just the message via pubsub)
| if utf == "\r": | ||
| pass # ignore | ||
| elif utf == "\n": | ||
| self._handleLogLine(self.cur_log_line) |
There was a problem hiding this comment.
I guess this is where we're handling log stuff that doesn't have a full LogRecord attached to it. I guess maybe the thing to do will just be to construct one with an "unset" log level. Over time more of this will move into proper protobufs anyway, I suppose
| 0x239a RAK4631 | ||
| 0x303a Heltec tracker""" | ||
| whitelistVids = dict.fromkeys([0x239a, 0x303a]) | ||
| whitelistVids = dict.fromkeys([0x239a, 0x303a, 0x04b4]) |
There was a problem hiding this comment.
What device is 0x04b4? Probably worth documenting so we don't lose what these are, I think
There was a problem hiding this comment.
heh. I don't know I'll look around. I had the same thought when I added my second vid ;-). I just tried plugging in a few different boards and didn't see it I'll check the commit history and hopefully I had a comment. ;-)
Yeah - my plan (sound okay to you @thebentern) is that the singleton Logger instance will do the formatting at the beginning and then be dispatching LogRecords to all of the observers who want to are handing out logs to their clients (serial, TCP, BLE)
yah - my hope is those changes (powermon/powerstress) will be pretty localized.
smart! |
Oh yes if you want to do that it would be awesome! It will allow me to focus on my power goo stuff anyways! |
Oh -- yeah, I only mean here on the python side of things, but from what little I understand of C++ that stuff sounds good on that end too. I just want to change |
0x04b4 is cypress semi but commonly used in Chinese oscopes (like mine). So it was supposed to be a blacklist not a whitelist!
|
ok - I think all good now (except for the codecoverage which I can ignore for now?) |
|
okay for this to go in @ianmcorvidae? I have a few more (much more localized) changes coming (finishing the power mon work items) - which I could either add to this PR or start a new one. Which do you prefer? |
|
Yeah, I'm sorry. Got slammed with a busy week and weekend and didn't get back to merging this, but I'll make a 2.3.14 release shortly and then merge this to master for 2.3.15, and update the protobufs. |
|
no rush - just checking! |
4 participants
Mostly done but I'd like to request review/merge soonish (because I've got too many open branches with changes and tire of merging ;-) ) - see bottom comment.
Fixes #603