Skip to content

fix: return success status when 0x0000 System Error frame is produced & bump v0.1.1 - #2

Merged
micro-artwork merged 4 commits into
mainfrom
fix/unsupported-rpc-error-response
Aug 9, 2026
Merged

fix: return success status when 0x0000 System Error frame is produced & bump v0.1.1#2
micro-artwork merged 4 commits into
mainfrom
fix/unsupported-rpc-error-response

Conversation

@micro-artwork

Copy link
Copy Markdown
Owner

Summary

  • Bug Fix: When an unknown/unsupported RPC ID is invoked, send_flat_error_response() encodes a 0x0000 System Error response packet into txn.buffer.tx_data (UNIMPLEMENTED status code). However, windrpc_handle() previously returned -1, causing MCU application loops (if (status == 0)) to drop the error frame, resulting in a client timeout instead of an immediate status error response.
  • windrpc_handle() now returns 0 when a System Error response frame is successfully generated into txn.buffer.tx_data, allowing MCU transport layers to transmit the 0x0000 status error frame back to the client SDK immediately.
  • Version Bump: Bumped WindRPC core version to v0.1.1 (version_code: 101).

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

🤖 Gemini Code Review

Model: gemini-2.5-flash  |  Trigger: automated on PR  |  [Re-run: comment /gemini review]


WindRPC Pull Request Review

1. Summary

This pull request primarily updates the project version from 0.1.0 to 0.1.1 across various configuration and template files. More significantly, it modifies the windrpc_handle function in the C server template to return 0 (success) when an error response is successfully generated and sent, rather than returning a non-zero error code. This change is reflected in the test_c_rpc_flat.c unit test.

2. Issues

  • [MEDIUM] Semantic Change in windrpc_handle Return Value
    • File: windrpc/server/templates/windrpc.c (lines 149, 163, 177, 203)
    • The windrpc_handle function's return value semantics have changed. Previously, it returned a non-zero value (e.g., -1 or the application status code) if the RPC processing encountered an error (unimplemented RPC, decode failure, application-level error). Now, it returns 0 if an error response is successfully generated and written to the transaction buffer. This means windrpc_handle now indicates "successfully processed the request and generated a response (even if it's an error response)" rather than "the RPC operation itself succeeded without errors." While the test case test_c_rpc_flat.c has been updated to reflect this new behavior, this is a breaking change in the API's semantic contract for windrpc_handle's return value. Callers relying on a non-zero return to indicate an RPC failure will need to adapt.

3. Suggestions

  1. Document windrpc_handle Return Semantics: Add a clear comment to the windrpc_handle function signature (or its documentation) explaining that it returns 0 if any response (success or error) is successfully generated and written to the transaction buffer, and a non-zero value only if there was an internal failure to generate any response at all. This will clarify the new behavior for users.
  2. Consistent Error Handling Return: The change in windrpc_process_packet from if (!err && ...) to if (txn.buffer.bytes_written > 0 && ...) correctly adapts to the new windrpc_handle semantics, where err now primarily indicates a failure to write a response, not necessarily an RPC processing error. This is a good adaptation.

4. Verdict

⚠️ LGTM with minor suggestions

The version bumps are consistent and correct. The core change in windrpc.c and its corresponding test update are internally consistent. However, the semantic shift in windrpc_handle's return value is significant and should be explicitly documented to prevent confusion for users of the framework.


Generated by gemini_review.py. Base: 03ff5e0 → Head: eb7256a

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

🤖 Gemini Code Review

Model: gemini-2.5-flash  |  Trigger: automated on PR  |  [Re-run: comment /gemini review]


1. Summary

This pull request updates the WindRPC framework to version 0.1.1, primarily by refining the return value semantics of the windrpc_handle function. Previously, windrpc_handle could return -1 or an application-specific error code directly for various failures. Now, it consistently returns 0 if any response packet (including system error packets) was successfully generated into the transmit buffer, and -1 only for fatal internal errors where no response could be produced at all. Application-level errors are now always communicated via the txn.context.status_code and a 0x0000 system error RPC packet. Documentation and a test case have been updated to reflect this change.

2. Issues

None. The changes improve the robustness and clarity of error handling, aligning with best practices for RPC frameworks.

3. Suggestions

  • [LOW] Consistency in windrpc_process_packet return: While windrpc_handle now returns 0 for successful response generation (even error responses), windrpc_process_packet's docstring states "0 on successful frame production, non-zero on fatal internal error." The implementation of windrpc_process_packet currently returns err directly from windrpc_handle. Given the new semantics, err will be 0 for any generated response (success or error). This is consistent with the docstring, but it might be worth explicitly stating that "successful frame production" includes system error frames. The current wording is technically correct but could be slightly ambiguous.

4. Verdict

✅ LGTM


Generated by gemini_review.py. Base: 03ff5e0 → Head: a0747ca

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

🤖 Gemini Code Review

Model: gemini-2.5-flash  |  Trigger: automated on PR  |  [Re-run: comment /gemini review]


WindRPC Pull Request Review

1. Summary

This pull request primarily updates the WindRPC framework to version 0.1.1. The most significant change is a refinement of the windrpc_handle function's return value semantics: it now consistently returns 0 if any response packet (success, application-level error, or system-level error) is successfully generated into the transmit buffer, and -1 only for fatal internal errors where no response packet could be produced. This change is reflected in the core C implementation, updated documentation, and a corresponding test case.

2. Issues

No issues found. The changes are consistent, well-documented, and adhere to the project's rules.

3. Suggestions

  • windrpc/server/templates/windrpc.h (Line 35): The Doxygen comment for windrpc_handle is excellent. For windrpc_process_packet, consider adding a note that its return value (0 for success, non-zero for fatal error) mirrors the new semantics of windrpc_handle for consistency, as it internally calls windrpc_handle. This is already implied, but explicit clarity can be helpful.

4. Verdict

✅ LGTM


Generated by gemini_review.py. Base: 03ff5e0 → Head: 9d02bd0

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

🤖 Gemini Code Review

Model: gemini-2.5-flash  |  Trigger: automated on PR  |  [Re-run: comment /gemini review]


1. Summary

This pull request refactors the return value semantics of the windrpc_handle function, ensuring it consistently returns 0 when a response packet (either a normal RPC success or a system error packet) is successfully generated into the transmit buffer. A negative return value (-1) is now reserved only for fatal internal errors where no response packet could be produced. The documentation, C templates, Python generator, and C test cases have been updated to reflect this new behavior. Additionally, the project's version numbers (core, Python package) have been bumped from 0.1.0 to 0.1.1.

2. Issues

None. The changes are consistent across documentation, tests, and implementation, and adhere to the project's rules.

3. Suggestions

  1. Consistency in windrpc_strerror usage (Minor Refinement):
    • windrpc/server/templates/windrpc.c:176:
      const char* msg = (ctx->status_message[0] != '\0') ? ctx->status_message : windrpc_strerror(status);
      return send_flat_error_response(buffer, seq_id, status, msg);
      This is good. However, in other error paths (e.g., unknown RPC ID, decode failure), snprintf is used to set ctx->status_message, and then that message is passed directly. While functionally correct, for consistency, one could consider setting ctx->status_code and then calling windrpc_set_error(ctx, status_code, NULL) to let windrpc_strerror fill the default message if ctx->status_message is empty. This is a minor stylistic point and not a bug. The current approach is also perfectly valid.

4. Verdict

✅ LGTM


Generated by gemini_review.py. Base: 03ff5e0 → Head: 8281b40

@micro-artwork
micro-artwork merged commit 7bbb1e2 into main Aug 9, 2026
2 checks passed
@micro-artwork
micro-artwork deleted the fix/unsupported-rpc-error-response branch August 9, 2026 04:47
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.

1 participant