Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fujitsu Printer Reconnect & Power Settings #4831

Merged
merged 4 commits into from
May 10, 2024
Merged

Conversation

adghayes
Copy link
Collaborator

@adghayes adghayes commented May 8, 2024

Commit 1: Currently VxScan just crashes if the printer is not attached on startup, and it cannot handle disconnects and reconnects. While this was OK for the happy path, it's not OK for the sad path and development. Changing the layers so that we are just checking for the device on every status call.

Commit 2: Finishes removing the speed setting (started in commit 1) and also specifies the print quality setting on printer connect. This is to set "Auto-Division" which we've found that we need to set to prevent spikes in power draw that go beyond our v4 PSU capabilities.

@adghayes adghayes marked this pull request as draft May 8, 2024 00:01
@adghayes adghayes marked this pull request as ready for review May 8, 2024 22:30
@adghayes adghayes requested a review from jonahkagan May 8, 2024 22:31
@adghayes
Copy link
Collaborator Author

adghayes commented May 8, 2024

@jonahkagan normally @kofi-q has been reviewing this integration but since he's out, passing you to. Setting up the reconnect logic we talked about.

@adghayes adghayes changed the title Fujitsu Printer Reconnect Logic Fujitsu Printer Reconnect & Power Settings May 8, 2024
Copy link
Collaborator

@jonahkagan jonahkagan May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in this file feels a little complex to me. There's a strategy for managing the connection state of the driver that's implemented across two methods: getDriver and getStatus, and then all of the other methods have to remember to start by calling getStatus (which feels a little brittle).

Could this code be reorganized such that in order to access the driver, each method needs to call some function that checks the connection status? A sketch:

async print(data: Buffer) {
  const driverResult = await getDriver();
  if (driverResult.isErr()) { // If the driver is disconnected or otherwise in a non-ready state
     return driverResult;
  }

  const printResult = await print(driverResult.ok(), data); 
  ...

(Ideally in this scheme, there would be no this.driver - the only way to access it would be through getDriver)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonahkagan I ended up not re-organizing the code, but dumbing down my assumptions. I have never seen the printer be unresponsive while still attached, which we've seen with scanners. As long as it's connected, it responds to commands. So within print and advancePaper, I'm going to assume that the driver exists and is available. getStatus is the only endpoint which handles connection and disconnection. It's not actually plausible that we would jump straight to invoke an action - the app should always be checking for device status before starting an action.

@adghayes adghayes requested a review from jonahkagan May 9, 2024 18:54
Comment on lines 62 to 68
if (!this.driver) {
this.driver = await this.getDriver();
// if we failed to initialize the driver, the device is likely not connected
if (!this.driver) {
return { state: 'error', type: 'disconnected' };
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also write as:

this.driver ??= await this.getDriver();
 // if we failed to initialize the driver, the device is likely not connected
if (!this.driver) {
  return { state: 'error', type: 'disconnected' };
}

which is slightly simpler for me to read

/**
* Initializes and returns a new driver instance.
*/
private async getDriver(): Promise<Optional<FujitsuThermalPrinterDriver>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to initializeDriver to be slightly more descriptive?

@adghayes adghayes enabled auto-merge (squash) May 10, 2024 00:34
@adghayes adghayes merged commit ca8cd04 into main May 10, 2024
62 checks passed
@adghayes adghayes deleted the drew/printer-reconnect branch May 10, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants