3
\$\begingroup\$

There is an interactive program, namely Microchip Debugger (Later called MDB), which I want to communicate with from NodeJS program. You can think about it as any console-based debugger - they all do the same thing, just with slightly different output/input expectations. This question is about the lowest layer here - launching MDB process and creating an async API, that will be easy to use later, that will abstract away all text processing required on MDB's stdout/stdin.

My ultimate goal for this project is to create a debugger extension for VS Code that will support MDB to program and debug PIC MCUs from VS Code, however this is not what this question is about.

The whole code is available on pastebin.

The code is working - successfully debugging a test executable in simulator. However I don't feel very confident about the design which could be much improved - this is my first attempt at this kind of problem and first big Javascipt/Typescript project, hence I ask for review from more experienced programmers. I would like to get feedback primarily on:

Error handling strategy

const ERRORS = {
    get PROCESS_NOT_RUNNING() { return new Error("Process not running"); },
    /* Omitted similar code */
    get DEVICE_RUNNING() { return new Error("Device running"); },
};

I created ERRORS object that is a factory for all errors that might happen. Later I would extend Error class with my own that would also carry some kind of error code (which would live inside an exported object/enum) that would allow the user of the API to differentiate between errors.

Design of the MdbWrapper class'es API

Example function headers:

  • public async start(timeoutMs: number = 60000) {
  • public async selectDevice(device: string, timeoutMs: number = 60000) {
  • public async breakpoint_address(address: number, passCount?: number, timeoutMs: number = 60000): Promise<BreakpointInfo> {

All functions either reject with error created by ERRORS factory or resolve with whatever is most appropriate or void for simple commands like start or selectDevice. But for most of them the same can be achieved by firing the function and waiting for event. Maybe a better pattern to follow would be creating the API as fire and forget and providing a public convenience function waitForEventsOrTimeout and making all actions fire a unique event upon completion this allowing the user of the API to stall if required.

I don't feel very comfortable with passing this timeoutMs parameter for each function although at the same time and I don't really feel comfortable with all this async and Promise styles. I feel like it's because the API should be shaped differently, are there some patterns that I could follow?

Design of the onData member function

This function is called on data event on processes stdout.

    private onData = (data: any) => {
        type Resolver<T> = {
            eventName: string,
            args?: (data: T) => any[],
        } | ((data: T) => void);

        interface ValueHandlerString {
            key: string;
            resolver: Resolver<string>;
        }
        interface ValueHandlerRegExp {
            key: RegExp;
            resolver: Resolver<RegExpExecArray>;
        }

        const valueHandlers: Array<ValueHandlerString | ValueHandlerRegExp> = [
            { key: '>', resolver: () => {
                if (! this._initialized) {
                    this._initialized = true;
                    this.emit(MDB_EVENTS.MDB_INITIALIZED);
                }
            }},
            { key: 'Programming target...', resolver: { eventName: MDB_EVENTS.DEVICE_PRORGRAMING_START } },
            { key: 'Program succeeded.', resolver: () => this._imageLoaded = true },
            { key: 'Program succeeded.', resolver: {eventName: MDB_EVENTS.DEVICE_PRORGRAMING_SUCCESS } },
            { key: 'Running', resolver: { eventName: MDB_EVENTS.DEVICE_RUNNING }},
            { key: 'Running', resolver: () => this._running = true },
            { key: 'HALTED', resolver: {
                eventName: MDB_EVENTS.DEVICE_HALT,
                args: () => [{...this._haltInfo, breakpoint: this._haltInfo.address === this._lastBreakpoint}]
            }},
            { key: 'HALTED', resolver: () => { this._running = false; this._lastBreakpoint = -1; } },
            { key: /^>?Working directory: (.*)$/, resolver: {eventName: MDB_EVENTS.MDB_CHANGE_DIRECTORY, args: (arr: RegExpExecArray) => [arr[1]]}},
            { key: /^>?(W[0-9]{4}-[A-Z]{3}): .*$/, resolver: {eventName: MDB_EVENTS.CONSOLE_WARNING, args: (arr: RegExpExecArray) => [arr[0], arr[1], arr[2]]} },
            { key: /^>?Single breakpoint: @0x([0-9A-F]+)$/, resolver: (arr: RegExpExecArray) => this._lastBreakpoint = parseInt(arr[1], 16) },
            { key: /^>?Breakpoint ([0-9]+) at 0x([0-9a-f]+).$/, resolver: {
                eventName: MDB_EVENTS.DEVICE_BREAKPOINT_SET,
                args: (arr: RegExpExecArray) => [{ id: parseInt(arr[1], 10), address: parseInt(arr[2], 16) }]
            }},
            { key: /^>?Breakpoint ([0-9]+) at function (.+)\.$/, resolver: {
                eventName: MDB_EVENTS.DEVICE_BREAKPOINT_SET,
                args: (arr: RegExpExecArray) => [{ id: parseInt(arr[1], 10), function: arr[2] }]
            }},
            { key: /^>?Breakpoint ([0-9]+) at file (.+), line ([0-9]+).$/, resolver: {
                eventName: MDB_EVENTS.DEVICE_BREAKPOINT_SET,
                args: (arr: RegExpExecArray) => [{ id: parseInt(arr[1], 10), file: { name: arr[2], line: parseInt(arr[3], 10) } }]
            }},
            { key: 'Stop at', resolver: () => this._haltInfo = {} },
            { key: /^>?address:0x([0-9a-f]+)$/, resolver: (matches: RegExpExecArray) => this._haltInfo.address = parseInt(matches[1], 16) },
            { key: /^>?file:(.+)$/, resolver: (matches: RegExpExecArray) => this._haltInfo.file = matches[1] },
            { key: /^>?source line:([0-9]+)$/, resolver: (matches: RegExpExecArray) => this._haltInfo.line = parseInt(matches[1], 10) },
            { key: /^>?---debug/, resolver: () => null } /* Silence "---debug peripheral accessed" - provide interface for it later */,
            { key: 'Simulator halted', resolver: () => null } /* Silence "Simulator halted" it's the same as HALTED */,
            { key: 'Stepping', resolver: () => this._running = true } /* Silence "Stepping" - it's output after "stepi" command */,
        ];

        const callResolver = <T>(resolver: Resolver<T>, arg: T) => {
            if (typeof resolver === 'function') {
                resolver(arg);
            } else {
                const args = resolver.args ? resolver.args(arg) : [];
                this.emit(resolver.eventName, ...args);
            }
        };

        const lines = String(data).split("\n").map(line => line.trim()).filter(line => line.length > 0);

        lines.forEach(line => {
            const altLine = (line[0] === '>') ? line.substring(1) : line;

            let handled = false;
            for (const handler of valueHandlers) {
                if (typeof handler.key === 'string') {

                    if (line !== handler.key) {
                        if (altLine === handler.key) {
                            callResolver((handler as ValueHandlerString).resolver, altLine);
                            handled = true;
                        }
                    } else {
                        callResolver((handler as ValueHandlerString).resolver, line);
                        handled = true;
                    }

                } else {

                    const match = handler.key.exec(line);
                    if (match) {
                        callResolver((handler as ValueHandlerRegExp).resolver, match);
                        handled = true;
                    }
                }
            }
            if (! handled) {
                console.warn("Unhandled line", line);
            }
        });
    }

This function encapsulates a lot of logic that boils down to:

  • For each line from stdout
  • Match it to each valueHandler either using regex or simple string compare. Note that MDB sometimes emits > symbol just before whatever output I'm looking for. So that needs to be accounted for.
  • Execute appropriate action - that is either simply emitting an event or executing a lambda.

Currently it's nearly 100 lines long function that contains many non-obvious inner functions and quite complex connections between it's internal data for my taste. I feel like it's too complicated to exist as a single unit, how could this be improved? Are there any patterns? My current choice would be to extract whole output parsing to a separate class, but at the same time I feel like it's not good approach in this case.

Or maybe it is okay to encapsulate all this logic inside a function in javascript and I'm just not used to this?

Also I hit a dead spot with the above as getHwTools member function would require a totally different parsing of MDB's output (a table) that just doesn't fit current design. I've already encountered it when parsing breakpoints witch resulted in creation of _haltInfo and _lastBreakpoint member variables, which form a separate state machine inside already overgrown object. It would be possible to implement (for example creating regexes that would match only this table or some global state determining current parsing mode), but I feel like it would just be going down the rabbit hole.

How to approach testing such code in Typescript?

I can't really use MDB for it as it is highly buggy and not deterministic (it has some concurrency on stdout issues, likes to spuriously hang and such). I though of mocking child_process and providing my own stdin and stdout stream that would simulate whatever it is that I want to test, but I don't know how to encapsulate it enough, so that test-cases would be short, easy to write and really obvious what they are testing. Are there some patterns.

I have a lot of member functions that look like this:

    public async stepIn(timeoutMs: number = 60000) {
        if (! this._imageLoaded) {
            throw ERRORS.IMAGE_NOT_LOADED;
        }

        if (this._running) {
            throw ERRORS.DEVICE_RUNNING;
        }

        this._running = true;
        this.writeLnAndWaitForEvent(MDB_COMMANDS.STEP(), MDB_EVENTS.DEVICE_HALT, timeoutMs);
    }

They all do the following:

  • Check some preconditions and throw appropriate error if not met
  • (sometimes) Set some internal state
  • Execute (write to MDBs stdin) a command and wait for some event that should be a result of it.

This probably can be simplified I just don't know how.

Remarks

I feel like most of my problem with design here is lack of separation of concerns. I've created this one big class which has the API that I want, but does everything inside instead of delegating tasks to other units (like parsing of the MDBs output). I've done this because I can't really find any good pattern for merging multiple EventEmmiters into one big class. So either I'm looking in wrong places or this should be solved differently - I would really appreciate input in this area.

As far as the review is concerned please disregard test function at the bottom - It's there to test the class during development and show example usage of the API.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Please revisit How do I ask a Good Question? and improve your question accordingly. \$\endgroup\$ – greybeard May 2 at 9:46
  • \$\begingroup\$ @greybeard is it better? What areas should I try to improve more? \$\endgroup\$ – Enbyted May 3 at 9:56
  • \$\begingroup\$ For the outstanding reason of deciding whether your question gets read or not, the first actionable paragraph in the help page linked is about titling: In my eyes, your title is too long. I used to do remote debugging as well as cross debugging and (once) ICE - and think the terms even more detached than in the day. You seem to want to debug "processes" on an MCU using support code there, VS Code locally (I think you can and should mark up this in the title, or tag vscode - can't seem to make up my mind). \$\endgroup\$ – greybeard May 4 at 7:16
  • \$\begingroup\$ In the introduction, you could try and unambiguously describe which parts are executing where (interactive console program on MCU (console?!), wrapper locally/in VS Code?). Questions not containing the lion's share of what there is to review are off topic. I see your question at the very verge (and do not intend to delve into the pastebin code to try and decide if there was a more appropriate line to draw, and where). \$\endgroup\$ – greybeard May 4 at 7:22
  • \$\begingroup\$ @greybeard I clarified the scope of this question. I think I focused too much on the end goal and not what the code actually does - which led to confusion. Also - thank you for taking your time and helping me improve the question. \$\endgroup\$ – Enbyted May 4 at 17:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.