From cf32519404ab824ae09a221d14ed8e4f317bd77c Mon Sep 17 00:00:00 2001 From: Carlos Chinchilla Date: Mon, 27 Nov 2023 22:27:07 +0000 Subject: [PATCH] pw_system: Style fixes to Python scripts - Move time_offset from Device to DeviceWithTracing. - Include modules not classes. While PEP8 allows including classes, it is easier to know where a class or free function comes from when reading the code -- this is also what the Google style suggest, which we don't follow but PEP8 allows it. - Fix multiline and single line strings. - Pass kwargs as kwargs in DeviceWithTracing. - Use only one type of quotes (single). Test: Ran pw_system demo on host and pw console Change-Id: I06d897283a3536e63b6ceee1485cef5b06e1c6a5 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/182661 Reviewed-by: Ted Pudlik Reviewed-by: Dave Roth Commit-Queue: Carlos Chinchilla --- pw_system/py/pw_system/console.py | 82 ++++++++++++++---------- pw_system/py/pw_system/device.py | 54 +++++++--------- pw_system/py/pw_system/device_tracing.py | 16 +++-- pw_system/py/pw_system/trace_client.py | 48 +++++++------- 4 files changed, 107 insertions(+), 93 deletions(-) diff --git a/pw_system/py/pw_system/console.py b/pw_system/py/pw_system/console.py index 6f6405cacf..227d45fdd1 100644 --- a/pw_system/py/pw_system/console.py +++ b/pw_system/py/pw_system/console.py @@ -55,17 +55,17 @@ import IPython # type: ignore from pw_cli import log as pw_cli_log -from pw_console.embed import PwConsoleEmbed -from pw_console.log_store import LogStore -from pw_console.plugins.bandwidth_toolbar import BandwidthToolbar -from pw_console.pyserial_wrapper import SerialWithLogging -from pw_console.python_logging import create_temp_log_file, JsonLogFormatter -from pw_console.socket_client import SocketClient, SocketClientWithLogging +from pw_console import embed +from pw_console import log_store +from pw_console.plugins import bandwidth_toolbar +from pw_console import pyserial_wrapper +from pw_console import python_logging +from pw_console import socket_client from pw_hdlc import rpc from pw_rpc.console_tools.console import flattened_rpc_completions -from pw_system.device import Device -from pw_system.device_tracing import DeviceWithTracing -from pw_tokenizer.detokenize import AutoUpdatingDetokenizer +from pw_system import device as pw_device +from pw_system import device_tracing +from pw_tokenizer import detokenize # Default proto imports: from pw_log.proto import log_pb2 @@ -165,8 +165,8 @@ def get_parser() -> argparse.ArgumentParser: 'Socket address used to connect to server. Type "default" to use ' 'localhost:33000, pass the server address and port as ' 'address:port, or prefix the path to a forwarded socket with ' - f'"{SocketClient.FILE_SOCKET_SERVER}:" as ' - f'{SocketClient.FILE_SOCKET_SERVER}:path_to_file.' + f'"{socket_client.SocketClient.FILE_SOCKET_SERVER}:" as ' + f'{socket_client.SocketClient.FILE_SOCKET_SERVER}:path_to_file.' ), ) parser.add_argument( @@ -265,10 +265,10 @@ def _expand_globs(globs: Iterable[str]) -> Iterator[Path]: def _start_python_terminal( # pylint: disable=too-many-arguments - device: Device, - device_log_store: LogStore, - root_log_store: LogStore, - serial_debug_log_store: LogStore, + device: pw_device.Device, + device_log_store: log_store.LogStore, + root_log_store: log_store.LogStore, + serial_debug_log_store: log_store.LogStore, log_file: str, host_logfile: str, device_logfile: str, @@ -327,14 +327,14 @@ def _start_python_terminal( # pylint: disable=too-many-arguments client_info = device.info() completions = flattened_rpc_completions([client_info]) - log_windows: Dict[str, Union[List[logging.Logger], LogStore]] = { + log_windows: Dict[str, Union[List[logging.Logger], log_store.LogStore]] = { 'Device Logs': device_log_store, 'Host Logs': root_log_store, } if serial_debug: log_windows['Serial Debug'] = serial_debug_log_store - interactive_console = PwConsoleEmbed( + interactive_console = embed.PwConsoleEmbed( global_vars=local_variables, local_vars=None, loggers=log_windows, @@ -344,7 +344,9 @@ def _start_python_terminal( # pylint: disable=too-many-arguments ) interactive_console.add_sentence_completer(completions) if serial_debug: - interactive_console.add_bottom_toolbar(BandwidthToolbar()) + interactive_console.add_bottom_toolbar( + bandwidth_toolbar.BandwidthToolbar() + ) # Setup Python logger propagation interactive_console.setup_python_logging( @@ -387,13 +389,13 @@ def console( # Don't send device logs to the root logger. _DEVICE_LOG.propagate = False - # Create pw_console LogStore handlers. These are the data source for log - # messages to be displayed in the UI. - device_log_store = LogStore() - root_log_store = LogStore() - serial_debug_log_store = LogStore() - # Attach the LogStores as handlers for each log window we want to show. - # This should be done before device initialization to capture early + # Create pw_console log_store.LogStore handlers. These are the data source + # for log messages to be displayed in the UI. + device_log_store = log_store.LogStore() + root_log_store = log_store.LogStore() + serial_debug_log_store = log_store.LogStore() + # Attach the log_store.LogStores as handlers for each log window we want to + # show. This should be done before device initialization to capture early # messages. _DEVICE_LOG.addHandler(device_log_store) _ROOT_LOG.addHandler(root_log_store) @@ -402,7 +404,7 @@ def console( if not logfile: # Create a temp logfile to prevent logs from appearing over stdout. This # would corrupt the prompt toolkit UI. - logfile = create_temp_log_file() + logfile = python_logging.create_temp_log_file() log_level = logging.DEBUG if verbose else logging.INFO @@ -445,7 +447,7 @@ def console( if json_logfile: json_filehandler = logging.FileHandler(json_logfile, encoding='utf-8') json_filehandler.setLevel(log_level) - json_filehandler.setFormatter(JsonLogFormatter()) + json_filehandler.setFormatter(python_logging.JsonLogFormatter()) _DEVICE_LOG.addHandler(json_filehandler) detokenizer = None @@ -454,7 +456,9 @@ def console( for token_database in token_databases: token_databases_with_domains.append(str(token_database) + "#trace") - detokenizer = AutoUpdatingDetokenizer(*token_databases_with_domains) + detokenizer = detokenize.AutoUpdatingDetokenizer( + *token_databases_with_domains + ) detokenizer.show_errors = True protos: List[Union[ModuleType, Path]] = list(_expand_globs(proto_globs)) @@ -490,7 +494,11 @@ def console( timestamp_decoder = None if socket_addr is None: - serial_impl = SerialWithLogging if serial_debug else serial.Serial + serial_impl = ( + pyserial_wrapper.SerialWithLogging + if serial_debug + else serial.Serial + ) serial_device = serial_impl( device, baudrate, @@ -510,9 +518,15 @@ def milliseconds_to_string(timestamp): timestamp_decoder = milliseconds_to_string else: - socket_impl = SocketClientWithLogging if serial_debug else SocketClient + socket_impl = ( + socket_client.SocketClientWithLogging + if serial_debug + else socket_client.SocketClient + ) - def disconnect_handler(socket_device: SocketClient) -> None: + def disconnect_handler( + socket_device: socket_client.SocketClient, + ) -> None: """Attempts to reconnect on disconnected socket.""" _LOG.error('Socket disconnected. Will retry to connect.') while True: @@ -535,17 +549,17 @@ def disconnect_handler(socket_device: SocketClient) -> None: return 1 with reader: - device_client = DeviceWithTracing( - ticks_per_second, + device_client = device_tracing.DeviceWithTracing( channel_id, reader, write, - protos, + proto_library=protos, detokenizer=detokenizer, timestamp_decoder=timestamp_decoder, rpc_timeout_s=5, use_rpc_logging=rpc_logging, use_hdlc_encoding=hdlc_encoding, + ticks_per_second=ticks_per_second, ) with device_client: _start_python_terminal( diff --git a/pw_system/py/pw_system/device.py b/pw_system/py/pw_system/device.py index 470ddc2368..8714212801 100644 --- a/pw_system/py/pw_system/device.py +++ b/pw_system/py/pw_system/device.py @@ -19,23 +19,13 @@ from typing import Any, Callable, List, Union, Optional from pw_thread_protos import thread_pb2 -from pw_hdlc.rpc import ( - HdlcRpcClient, - channel_output, - NoEncodingSingleChannelRpcClient, - RpcClient, - CancellableReader, -) -from pw_log.log_decoder import ( - Log, - LogStreamDecoder, - log_decoded_log, - timestamp_parser_ns_since_boot, -) -from pw_log_rpc.rpc_log_stream import LogStreamHandler +from pw_hdlc import rpc +from pw_log import log_decoder +from pw_log_rpc import rpc_log_stream from pw_metric import metric_parser -from pw_rpc import callback_client, Channel, console_tools -from pw_thread.thread_analyzer import ThreadSnapshotAnalyzer +import pw_rpc +from pw_rpc import callback_client, console_tools +from pw_thread import thread_analyzer from pw_tokenizer import detokenize from pw_tokenizer.proto import decode_optionally_tokenized from pw_unit_test.rpc import run_tests as pw_unit_test_run_tests, TestRecord @@ -53,18 +43,15 @@ class Device: Note: use this class as a base for specialized device representations. """ - # pylint: disable=too-many-instance-attributes def __init__( - # pylint: disable=too-many-arguments self, channel_id: int, - reader: CancellableReader, + reader: rpc.CancellableReader, write, proto_library: List[Union[ModuleType, Path]], detokenizer: Optional[detokenize.Detokenizer] = None, timestamp_decoder: Optional[Callable[[int], str]] = None, rpc_timeout_s: float = 5, - time_offset: int = 0, use_rpc_logging: bool = True, use_hdlc_encoding: bool = True, logger: logging.Logger = DEFAULT_DEVICE_LOGGER, @@ -73,7 +60,6 @@ def __init__( self.protos = proto_library self.detokenizer = detokenizer self.rpc_timeout_s = rpc_timeout_s - self.time_offset = time_offset self.logger = logger self.logger.setLevel(logging.DEBUG) # Allow all device logs through. @@ -96,10 +82,12 @@ def detokenize_and_log_output(data: bytes, _detokenizer=None): for line in log_messages.splitlines(): self.logger.info(line) - self.client: RpcClient + self.client: rpc.RpcClient if use_hdlc_encoding: - channels = [Channel(self.channel_id, channel_output(write))] - self.client = HdlcRpcClient( + channels = [ + pw_rpc.Channel(self.channel_id, rpc.channel_output(write)) + ] + self.client = rpc.HdlcRpcClient( reader, self.protos, channels, @@ -107,8 +95,8 @@ def detokenize_and_log_output(data: bytes, _detokenizer=None): client_impl=callback_client_impl, ) else: - channel = Channel(self.channel_id, write) - self.client = NoEncodingSingleChannelRpcClient( + channel = pw_rpc.Channel(self.channel_id, write) + self.client = rpc.NoEncodingSingleChannelRpcClient( reader, self.protos, channel, @@ -118,22 +106,22 @@ def detokenize_and_log_output(data: bytes, _detokenizer=None): if use_rpc_logging: # Create the log decoder used by the LogStreamHandler. - def decoded_log_handler(log: Log) -> None: - log_decoded_log(log, self.logger) + def decoded_log_handler(log: log_decoder.Log) -> None: + log_decoder.log_decoded_log(log, self.logger) - self._log_decoder = LogStreamDecoder( + self._log_decoder = log_decoder.LogStreamDecoder( decoded_log_handler=decoded_log_handler, detokenizer=self.detokenizer, source_name='RpcDevice', timestamp_parser=( timestamp_decoder if timestamp_decoder - else timestamp_parser_ns_since_boot + else log_decoder.timestamp_parser_ns_since_boot ), ) # Start listening to logs as soon as possible. - self.log_stream_handler = LogStreamHandler( + self.log_stream_handler = rpc_log_stream.LogStreamHandler( self.rpcs, self._log_decoder ) self.log_stream_handler.listen_to_logs() @@ -184,5 +172,7 @@ def snapshot_peak_stack_usage(self, thread_name: Optional[str] = None): for thread_info_block in rsp: for thread in thread_info_block.threads: thread_info.threads.append(thread) - for line in str(ThreadSnapshotAnalyzer(thread_info)).splitlines(): + for line in str( + thread_analyzer.ThreadSnapshotAnalyzer(thread_info) + ).splitlines(): _LOG.info('%s', line) diff --git a/pw_system/py/pw_system/device_tracing.py b/pw_system/py/pw_system/device_tracing.py index c97dabe081..c10b3abcae 100644 --- a/pw_system/py/pw_system/device_tracing.py +++ b/pw_system/py/pw_system/device_tracing.py @@ -37,7 +37,7 @@ class DeviceWithTracing(Device): """Represents an RPC Client for a device running a Pigweed target with tracing. - The target must have and RPC support for the following services: + The target must have RPC support for the following services: - logging - file - transfer @@ -45,8 +45,14 @@ class DeviceWithTracing(Device): Note: use this class as a base for specialized device representations. """ - def __init__(self, ticks_per_second: Optional[int], *argv, **kargv): - super().__init__(*argv, **kargv) + def __init__( + self, + *device_args, + ticks_per_second: Optional[int] = None, + time_offset: int = 0, + **device_kwargs, + ): + super().__init__(*device_args, **device_kwargs) # Create the transfer manager self.transfer_service = self.rpcs.pw.transfer.Transfer @@ -56,6 +62,7 @@ def __init__(self, ticks_per_second: Optional[int], *argv, **kargv): initial_response_timeout_s=self.rpc_timeout_s, default_protocol_version=pw_transfer.ProtocolVersion.LATEST, ) + self.time_offset = time_offset if ticks_per_second: self.ticks_per_second = ticks_per_second @@ -69,8 +76,7 @@ def get_ticks_per_second(self) -> int: resp = trace_service.GetClockParameters() if not resp.status.ok(): _LOG.error( - 'Failed to get clock parameters: %s. Using default \ - value', + 'Failed to get clock parameters: %s. Using default value', resp.status, ) return DEFAULT_TICKS_PER_SECOND diff --git a/pw_system/py/pw_system/trace_client.py b/pw_system/py/pw_system/trace_client.py index 575659b262..2dfa8d1e06 100644 --- a/pw_system/py/pw_system/trace_client.py +++ b/pw_system/py/pw_system/trace_client.py @@ -38,9 +38,9 @@ import pw_transfer from pw_file import file_pb2 from pw_hdlc import rpc -from pw_system.device_tracing import DeviceWithTracing -from pw_tokenizer.detokenize import AutoUpdatingDetokenizer -from pw_console.socket_client import SocketClient +from pw_system import device_tracing +from pw_tokenizer import detokenize +from pw_console import socket_client _LOG = logging.getLogger('pw_console_trace_client') @@ -87,21 +87,23 @@ def _parse_args(): '--baudrate', type=int, default=115200, - help='the baud rate to use', + help='The baud rate to use', ) group.add_argument( '-s', '--socket-addr', type=str, - default="default", - help='use socket to connect to server, type default for\ - localhost:33000, or manually input the server address:port', + default='default', + help=( + 'Use socket to connect to server, type default for ' + 'localhost:33000, or manually input the server address:port' + ), ) parser.add_argument( '-o', '--trace_output', dest='trace_output_file', - help=('The json file to which to write the output.'), + help='The json file to which to write the output.', ) parser.add_argument( '-t', @@ -113,30 +115,32 @@ def _parse_args(): '--ticks_per_second', type=int, dest='ticks_per_second', - help=('The clock rate of the trace events.'), + help='The clock rate of the trace events.', ) parser.add_argument( '--time_offset', type=int, dest='time_offset', default=0, - help=('Time offset (us) of the trace events (Default 0).'), + help='Time offset (us) of the trace events (Default 0).', ) parser.add_argument( '--channel-id', type=int, dest='channel_id', default=rpc.DEFAULT_CHANNEL_ID, - help="Channel ID used in RPC communications.", + help='Channel ID used in RPC communications.', ) return parser.parse_args() def _main(args) -> int: - detokenizer = AutoUpdatingDetokenizer(args.trace_token_database + "#trace") + detokenizer = detokenize.AutoUpdatingDetokenizer( + args.trace_token_database + '#trace' + ) detokenizer.show_errors = True - socket_impl = SocketClient + socket_impl = socket_client.SocketClient try: socket_device = socket_impl(args.socket_addr) reader = rpc.SelectableReader(socket_device) @@ -153,8 +157,7 @@ def _main(args) -> int: ] with reader: - device_client = DeviceWithTracing( - args.ticks_per_second, + device_client = device_tracing.DeviceWithTracing( args.channel_id, reader, write, @@ -164,17 +167,18 @@ def _main(args) -> int: rpc_timeout_s=5, use_rpc_logging=True, use_hdlc_encoding=True, + ticks_per_second=args.ticks_per_second, ) with device_client: - _LOG.info("Starting tracing") + _LOG.info('Starting tracing') start_tracing_on_device(device_client) - _LOG.info("Stopping tracing") + _LOG.info('Stopping tracing') file_id = stop_tracing_on_device(device_client) - _LOG.info("Trace file id = %d", file_id.response.file_id) + _LOG.info('Trace file id = %d', file_id.response.file_id) - _LOG.info("Listing Files") + _LOG.info('Listing Files') stream_response = list_files_on_device(device_client) if not stream_response.status.ok(): @@ -183,7 +187,7 @@ def _main(args) -> int: for list_response in stream_response.responses: for file in list_response.paths: - _LOG.info("Transfering File: %s", file.path) + _LOG.info('Transfering File: %s', file.path) try: data = device_client.transfer_manager.read(file.file_id) events = trace_tokenized.get_trace_events( @@ -199,10 +203,10 @@ def _main(args) -> int: except pw_transfer.Error as err: print('Failed to read:', err.status) - _LOG.info("Deleting File: %s", file.path) + _LOG.info('Deleting File: %s', file.path) delete_file_on_device(device_client, file.path) - _LOG.info("All trace transfers completed successfully") + _LOG.info('All trace transfers completed successfully') return 0