Skip to content

fix(ALU): correct borrow flag for subtraction and handle CTR=3 stale …#846

Open
Me-Priyank wants to merge 1 commit intoCircuitVerse:mainfrom
Me-Priyank:Priyank-export
Open

fix(ALU): correct borrow flag for subtraction and handle CTR=3 stale …#846
Me-Priyank wants to merge 1 commit intoCircuitVerse:mainfrom
Me-Priyank:Priyank-export

Conversation

@Me-Priyank
Copy link
Contributor

@Me-Priyank Me-Priyank commented Jan 22, 2026

Fixes #845

Describe the changes you have made in this PR -

This PR fixes two bugs in the ALU component:

Bug 1: Subtraction Borrow Flag Always Zero

When performing subtraction (A-B, Control Signal = 6), the Carry/Borrow output was hardcoded to 0. In real hardware, the borrow flag should be 1 when B > A to indicate that a borrow occurred.

Fix: Changed from

this.carryOut.value = 0

to

this.carryOut.value = +(this.inp1.value < this.inp2.value)

Bug 2: Control Signal 3 Outputs Stale Data

When Control Signal = 3, the ALU only updated the display message to "ALU" but did not update the output value. This caused the output to retain its previous value (stale data).

Fix: Added proper output handling that sets output to 0 and adds both output and carry to the simulation queue.

Files Modified:

  • src/simulator/src/modules/ALU.js
  • v1/src/simulator/src/modules/ALU.js

Screenshots of the UI changes (If any) -

Bug 1 fixed :

image

Bug 2 fixed :

image

Code Understanding and AI Usage

Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?

  • No, I wrote all the code myself
  • Yes, I used AI assistance (continue below)

If you used AI assistance:

  • I have reviewed every single line of the AI-generated code
  • I can explain the purpose and logic of each function/component I added
  • I have tested edge cases and understand how the code handles them
  • I have modified the AI output to follow this project's coding standards and conventions

Explain your implementation approach:

Problem: The ALU component had two issues affecting simulation correctness:

  1. Subtraction never set the borrow flag, making it impossible for downstream circuits to detect underflow
  2. Control signal 3 left stale data on the output bus

Alternative approaches considered:

  • For Bug 1: Could have used this.inp2.value > this.inp1.value but +(a < b) is more idiomatic JavaScript
  • For Bug 2: Could have set output to undefined, but 0 is more consistent with hardware behavior (no operation = zero output)

Why this implementation:

  • Minimal code changes (2 files, ~6 lines each)
  • Follows existing patterns in the codebase (other operations use similar carry flag logic)
  • Maintains backward compatibility (existing circuits will work, just with correct borrow behavior now)

Key changes:

  • this.carryOut.value = +(this.inp1.value < this.inp2.value) - Sets borrow flag to 1 when B > A
  • Added simulationQueue.add() calls for CTR=3 to properly propagate output changes

Checklist before requesting a review

  • I have added proper PR title and linked to the issue
  • I have performed a self-review of my code
  • I can explain the purpose of every function, class, and logic block I added
  • I understand why my changes work and have tested them thoroughly
  • I have considered potential edge cases and how my code handles them
  • If it is a core feature, I have added thorough tests
  • My code follows the project's style guidelines and conventions

Note: Please check Allow edits from maintainers if you would like us to assist in the PR.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed the no-operation instruction to properly clear outputs and set both result and carry flags to zero.
    • Fixed the subtraction operation to correctly handle the borrow flag, now setting it to indicate when borrowing is required.

✏️ Tip: You can customize this high-level summary in your review settings.

@netlify
Copy link

netlify bot commented Jan 22, 2026

Deploy Preview for circuitverse ready!

Name Link
🔨 Latest commit 9731ab9
🔍 Latest deploy log https://app.netlify.com/projects/circuitverse/deploys/6972371044613600086f501d
😎 Deploy Preview https://deploy-preview-846--circuitverse.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 33 (🔴 down 9 from production)
Accessibility: 73 (no change from production)
Best Practices: 92 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 22, 2026

Walkthrough

The ALU.js module is updated to fix two operational bugs. When control signal is 3 (no-operation), the module now sets output and carryOut to 0 and enqueues both values instead of only updating the display message. When control signal is 6 (subtraction A-B), the module now correctly sets carryOut to indicate a borrow by computing whether inp1 is less than inp2, using arithmetic coercion to convert the result to 1 or 0, rather than always setting carryOut to 0. These modifications ensure proper output handling for the no-operation mode and accurate borrow flag representation during subtraction operations.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly identifies the two main bugs being fixed: borrow flag for subtraction and stale output handling for CTR=3, which directly aligns with the changeset.
Linked Issues check ✅ Passed The PR successfully implements both required fixes from issue #845: corrects borrow flag logic for subtraction and clears stale outputs for CTR=3 control signal.
Out of Scope Changes check ✅ Passed All changes in both ALU.js files are directly related to the two bugs specified in issue #845; no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Me-Priyank
Copy link
Contributor Author

@Nihal4777 @tachyons kindly review

@Me-Priyank
Copy link
Contributor Author

@Nihal4777

@Me-Priyank
Copy link
Contributor Author

@Nihal4777 can you please review

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.

🐞 Bug: ALU Component - Subtraction Borrow Flag Always Zero and Control Signal 3 Outputs Stale Data

1 participant