refactor: remote failures provider #6971

Benjozork · 2022-03-26T00:37:10Z

Summary of Changes

In order to support a remote instance of the flyPad, global failure state needs to be available across multiple instruments.

Since the FailuresOrchestrator is meant to be unique across all instruments, the one in the flyPad present in the cockpit must remain the coordinator of failure state and control.

This patch introduces an interface, FailuresProvider, which is implemented by both FailuresOrchestrator and a new RemoteFailuresProvider. RemoteFailuresProvider instances can be unlimited in numbers and recieve / send data from and to the FailuresOrchestrator via Coherent events. It recieves state updates and asks the orchestrator to activate or deactivate failures when a user interaction is made.

Testing instructions

Make sure Failures page on the EFB still works.

How to download the PR for QA

Every new commit to this PR will cause a new A32NX artifact to be created, built, and uploaded.

Make sure you are signed in to GitHub
Click on the Checks tab on the PR
On the left side, click on the bottom PR tab
Click on the A32NX download link at the bottom of the page

tracernz

LGTM.... Would be nice for @davidwalschots to take a look as the architect of the system.

tracernz · 2022-04-02T08:08:17Z

jest/ViewListener.ts

@@ -0,0 +1,16 @@
+export let listeners = {};


/external/jest/ViewListener.ts 1:1 error Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

tracernz · 2022-04-02T08:09:00Z

src/instruments/src/EFB/index.tsx

@@ -1,6 +1,5 @@
 import React, { useState } from 'react';
-import { MemoryRouter as Router } from 'react-router-dom';


/external/src/instruments/src/EFB/index.tsx 66:14 error 'Router' is not defined react/jsx-no-undef

davidwalschots · 2022-04-03T09:05:19Z

docs/a320-coherent-triggers.md

+    - Dataflow: Failures orchestrator -> Remote failures providers
+    - Notifies any remote failures providers of available, active and changing failures
+    - Payload: `[active: number[], changing: number[]]`
+        - `all`: every failure currently active


I'm guessing this should be active?

Suggested change

- `all`: every failure currently active

- `active`: every failure currently active

davidwalschots · 2022-04-03T09:14:18Z

docs/a320-coherent-triggers.md

+- CONFIRM_ACTIVATE_FAILURE
+    - Dataflow: Failures orchestrator -> Remote failures providers
+    - Confirms to remote providers that a failure was activated
+    - Payload: `[identifier: number]`
+    - `identifier`: failure identifier
+
+- CONFIRM_DEACTIVATE_FAILURE


Suggested change

- CONFIRM_ACTIVATE_FAILURE

- Dataflow: Failures orchestrator -> Remote failures providers

- Confirms to remote providers that a failure was activated

- Payload: `[identifier: number]`

- `identifier`: failure identifier

- CONFIRM_DEACTIVATE_FAILURE

- A32NX_CONFIRM_ACTIVATE_FAILURE

- Dataflow: Failures orchestrator -> Remote failures providers

- Confirms to remote providers that a failure was activated

- Payload: `[identifier: number]`

- `identifier`: failure identifier

- A32NX_CONFIRM_DEACTIVATE_FAILURE

davidwalschots · 2022-04-03T09:14:33Z

jest/ViewListener.ts

+            }
+        }
+    }),
+};


Suggested change

};

};

davidwalschots · 2022-04-03T09:17:54Z

jest/ViewListener.ts

+    triggerToAllSubscribers: jest.fn((event, ...args) => {
+        const subs = listeners[event];
+
+        const jsonArgs = args.map((it) => JSON.stringify(it)).map((it) => JSON.parse(it));


thought: You copy the arguments. If your goal is to protect the caller of triggerToAllSubscribers from suddenly having its own objects changed under him, then this works. If your goal is for every individual subscriber to have their own copy of objects, then this will need to move within the loop.

The goal here is to reproduce what MSFS does (value -> json -> value), and lose the prototypes and object identities.

davidwalschots · 2022-04-03T09:19:53Z

jest/setupJestMock.js

+    triggerToAllSubscribers: jest.fn((event, ...args) => {
+        const subs = listeners[event];
+
+        const jsonArgs = args.map((it) => JSON.stringify(it)).map((it) => JSON.parse(it));


thought: You copy the arguments. If your goal is to protect the caller of triggerToAllSubscribers from suddenly having its own objects changed under him, then this works. If your goal is for every individual subscriber to have their own copy of objects, then this will need to move within the loop.

The goal here is to reproduce what MSFS does (value -> json -> value), and lose the prototypes and object identities.

davidwalschots · 2022-04-03T09:20:07Z

jest/setupJestMock.js

+});
+
+/** @type {Partial<ViewListener.ViewListener>} */
+const MockViewListener = {


question: Is the code duplicate intentional?

No. The TS file wasn't meant to be committed.

davidwalschots · 2022-04-03T09:23:23Z

src/failures/src/test-values.ts

+    return new Promise((resolve) => setImmediate(resolve));
+}
+
+const prefix = 'PREFIX';


thought: I'd be very careful about putting all these items in one file. I deliberately chose not to, because understanding tests where everything can be found locally is easier.
suggestion: Move code back to their original position.

davidwalschots · 2022-04-03T09:30:47Z

src/failures/src/failures-orchestrator.ts

 */
-export class FailuresOrchestrator {
+export class FailuresOrchestrator implements FailuresProvider {


thought: I have the feeling we're missing some abstractions in these changes. Now this type needs to know about Coherent's on, RegisterViewListener, and publishing. Maybe there's some additional work here?

davidwalschots · 2022-04-03T09:34:08Z

src/failures/src/remote-failures-provider.spec.ts

+        const o = orchestrator();
+        const rfp = remoteProvider();
+
+        await deactivateFailure(o);
+
+        expect(rfp.isActive(identifier)).toBe(false);


suggestion: First bring the test in a state where the failure is active in the rfp, then remove the failure and assert.

davidwalschots · 2022-04-03T09:34:55Z

src/failures/src/remote-failures-provider.spec.ts

+        const o = orchestrator();
+        const rfp = remoteProvider();
+
+        await remoteDeactivateFailure(o, rfp);
+
+        expect(o.isActive(identifier)).toBe(false);


suggestion: As above.

Benjozork · 2022-04-11T06:15:58Z

Thanks for the review. Will be looking at those shortly.

Benjozork · 2023-12-16T01:13:03Z

This will need to be redone because of #7821 anyway

Benjozork marked this pull request as draft March 26, 2022 00:38

Benjozork added 3 commits April 1, 2022 20:58

refactor: remote failures provider

1838b9c

fix and add tests

ca4f705

use more resilient method of confirming activation / deactivation

13c4887

Benjozork force-pushed the remote-failures-provider branch from 8697fde to 13c4887 Compare April 2, 2022 00:58

Benjozork marked this pull request as ready for review April 2, 2022 00:59

Benjozork requested a review from davidwalschots April 2, 2022 01:00

add missing docs

902829c

tracernz reviewed Apr 2, 2022

View reviewed changes

davidwalschots reviewed Apr 3, 2022

View reviewed changes

tracernz added this to the v0.8.0 milestone Apr 5, 2022

beheh removed this from the v0.8.0 milestone Apr 29, 2022

2hwk added this to the v0.9.0 milestone Apr 29, 2022

2hwk modified the milestones: v0.9.0, Backlog Aug 6, 2022

Benjozork closed this Dec 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remote failures provider #6971

refactor: remote failures provider #6971

Benjozork commented Mar 26, 2022

tracernz left a comment

tracernz Apr 2, 2022

tracernz Apr 2, 2022

davidwalschots Apr 3, 2022

davidwalschots Apr 3, 2022

davidwalschots Apr 3, 2022

davidwalschots Apr 3, 2022

Benjozork Apr 24, 2022

davidwalschots Apr 3, 2022

Benjozork Apr 24, 2022

davidwalschots Apr 3, 2022

Benjozork Apr 24, 2022

davidwalschots Apr 3, 2022

davidwalschots Apr 3, 2022

davidwalschots Apr 3, 2022

davidwalschots Apr 3, 2022

Benjozork commented Apr 11, 2022

Benjozork commented Dec 16, 2023

		@@ -1,6 +1,5 @@
		import React, { useState } from 'react';
		import { MemoryRouter as Router } from 'react-router-dom';

	- `all`: every failure currently active
	- `active`: every failure currently active

refactor: remote failures provider #6971

refactor: remote failures provider #6971

Conversation

Benjozork commented Mar 26, 2022

Summary of Changes

Testing instructions

How to download the PR for QA

tracernz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjozork commented Apr 11, 2022

Benjozork commented Dec 16, 2023