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

Feature Request - Implement dynamic file sizes and protocol versioning in PKSM bridge #1305

Open
mrhappyasthma opened this issue Nov 5, 2021 · 11 comments · May be fixed by #1327
Open

Feature Request - Implement dynamic file sizes and protocol versioning in PKSM bridge #1305

mrhappyasthma opened this issue Nov 5, 2021 · 11 comments · May be fixed by #1327
Labels
enhancement For any feature requests

Comments

@mrhappyasthma
Copy link
Contributor

Using the sword/shield bridge from the Switch Checkpoint transfers the data correctly. But trying to go to 'storage' after receiving the save is crashing.

Based on the change log, it looks like support up to v1.2 was added. Some work might need to be done after the latest updates.

Attached the Luma3DS crash log, in case it's helpful
crash_dump.txt
crash_dump_00000000.zip

@piepie62
Copy link
Member

piepie62 commented Nov 5, 2021

This is a known problem with the SWSH bridge that needs a redesign of the bridge itself on both Checkpoint and PKSM's sides. The essential issue is that save sizes change between updates, which our current bridge system was never designed for. Thank you for the report, but yeah, already known

@mrhappyasthma
Copy link
Contributor Author

Is it as simple as adding some kind of header data on the TCP connection specifying the incoming file size? Or is there significant amounts of work that needs to be done in addition to the size change?

(If it's not anything too unreasonable, I could take a stab at it. But I haven't touched these projects before so I'm coming in blind.)

@piepie62
Copy link
Member

piepie62 commented Nov 5, 2021

That would be the general idea, yeah; I'd also want the port to change in order to prevent improper format misreading, and probably a bit of versioning info. If you'd like to take a shot, be my guest; I hate networking code with a burning passion so it would be very appreciated

@mrhappyasthma
Copy link
Contributor Author

Just updating the save file size 'works'. Although all of the sprites for the SWSH pokemon show as an egg. (Assuming this worked before, perhaps its some issue with data offsets?)

But yeah, I can try to take a stab at rewriting/improving some of the networking logic for this.

@piepie62
Copy link
Member

piepie62 commented Nov 5, 2021

So, updating the save file size "works", yes, but a major problem with that is that there are several valid SWSH save file sizes, based off of the order/amount of updates taken. As for sprites, that's normal; we don't have properly sized G8 sprites so I haven't built a sheet that includes any yet.

@mrhappyasthma
Copy link
Contributor Author

Ah okay. Well if sprites are an external issue, looks like we just need to update the t3x file.

Adding support for versioning and sending/receiving files (with dynamic size) is definitely something I can look into it.

@mrhappyasthma mrhappyasthma changed the title Bug Report - Crash when transferring SWSH save (v1.3.2) Feature Request - Implement dynamic file sizes and protocol versioning in PKSM bridge Nov 5, 2021
@piepie62
Copy link
Member

piepie62 commented Nov 5, 2021

Yeah, sprites are just mostly a "I don't do graphic design well, so trying to shrink the sprites and not have them look like ass is a bad thing to assign to me" thing. Same main thing a PKSM Switch version is stuck on, really.

@mrhappyasthma
Copy link
Contributor Author

I can't help much with the assets, but i can take a look at the current TCP communication and try to whip up a reasonable protocol to make it a bit more robust to change and compliant with dynamic file sizes

@GriffinG1 GriffinG1 added the enhancement For any feature requests label Nov 5, 2021
@piepie62
Copy link
Member

piepie62 commented Nov 5, 2021

Yep, and thank you for that; was just explaining why it wasn't quite as simple as just updating the spritesheets

@mrhappyasthma
Copy link
Contributor Author

So I was thinking something like this. Let me know if anyone has suggestions or concerns.

I tried to keep it fairly basic, while still allowing for some flexibility to add more error codes or upgrading protocol versions in the future.

One other thing that I wasn't sure of: the protocol is fairly self-contained. Did we want to consider sharing this code somewhere that makes it easy to import? Then other projects that use this protocol (e.g. Checkpoint or even folks who wants to write custom tooling) could just include this directly without needing to re-defining all the structs and enums. I wasn't sure if there was any preference of file location for this to make it easier to share. (If we really wanted to go extreme, this could be in its own lightweight dependency. But even within the structure of PKSM, I wasn't entirely sure where to put it. I could just keep it in utils but then it felt kind of undiscoverable.)

Let me know what folks think.

/**
 * This file defines the PKSM Bridge protocol API.
 *
 * Example data flow:
 *
 *     Client -> Send `pksmBridgeRequest_s` to server and await acknowledgement of supported version
 *
 *     Server: Checks the protocol version from the request to see if it is supported.
 *
 *             <- Server send `pksmBridgeResponse_s`. If version is supported, the value matches
 *                one sent from the client. If unsupported, then the value is
 *                PKSM_BRIDGE_UNSUPPORTED_PROTOCOL_VERSION.
 *
 *     Client: Parses the `pksmBridgeResponse_s` to expect either an error payload or a file payload.
 *
 *             <- (if error) Server then sends `pksmBridgeError_s`
 *             <- (if !error) Server then sends `pksmBridgeFile_s`
 *
 *     Client: Conditionally receives the appropriate payload. And handles it appropriately.
 */
#ifndef PKSMBRIDGE_API_HPP
#define PKSMBRIDGE_API_HPP

const constexpr int PKSM_BRIDGE_LATEST_PROTOCOL_VERSION = 1;
const constexpr int PKSM_BRIDGE_UNSUPPORTED_PROTOCOL_VERSION = -1;

typedef struct {
    /** Used as a sentinel value to identify the request type. Contains 'PKSMBridge'. */
    char protocol_name[10];
    /** The requested protocol version to use. */ 
    s8 protocol_version;  
} pksmBridgeRequest_s;

typedef struct {
    /** Used as a sentinel value to identify the response type. Contains 'PKSMBridge'. */
    char protocol_name[10];
    /**
     * If the requested protocol_version is supported by the server, this value will
     * contain the same value to confirm support. If the requested version is not
     * supported, it will contain `PKSM_BRIDGE_UNSUPPORTED_PROTOCOL_VERSION`.
     */ 
    s8 protocol_version;
} pksmBridgeResponse_s;

namespace pksm {

/** An enum to define the various types of protocol errors. */
enum class BridgeErrorEnum : s16 {
    /**
     * This error code is used when a client requests a protocol version
     * that the server does not support.
     */
    UNSUPPORTED_VERSION = -1
};

}  // namespace pksm

typedef struct {
  /** The error code ID. */
  pksm::BridgeErrorEnum error_code;
  
  /** The size (in bytes) of the text in `error_message`. */
  u32 error_message_size;
  
  /** A UTF-8 encoded error message. */
  char *error_message;
} pksmBridgeError_s;

typedef struct {
  /** The size of the file in bytes. */
  u64 file_size;
  
  /**
   * The file contents themselves. The size of these contents is specified
   * in `file_size`.
   */
  char *file_contents;
} pksmBridgeFile_s;

#endif  // PKSMBRIDGE_HPP

@mrhappyasthma
Copy link
Contributor Author

mrhappyasthma commented Nov 6, 2021

Discussed offline.

Summary:

  • Prefer camelcase
  • Use clangformat for styling
  • Don't typedef the structs. Global namespace is fine.
  • Remove the _s suffix
  • Move BridgeErrorEnum into the global namespace
  • Fix the const constexpr redundancy
  • Drop pksmBridgeError_s for v1 of protocol. Having a string field means we have to worry about translations. It's also not necessary with a single error case, so let's simplify.
  • Sharing code for the protocol probably makes sense but may not be highest priority. (for now i'll just plan to implement in both places and take a TODO to refactor it out).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For any feature requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants