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

RAM negated enable #1855

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cosineblast
Copy link
Contributor

@cosineblast cosineblast commented Sep 10, 2023

This PR contains an implementation for a negated output and write enable for the RAM component as requested in issue #1510.
The implementation considers errors/unknowns as false values when the invert enable is activated (so it does enable read/write). When the output/write enable invert is on, a little circle shows up in the connection, just like the clock does.

Copy link
Member

@davidhutchens davidhutchens left a comment

Choose a reason for hiding this comment

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

I made some suggestions for minor cleanup.

Note to @BFH-ktt1: This adds memory with inverted enables to the list of things that are not supported in hdl. That doesn't break anything that currently exists, but should the hdl generation be enhanced to cover these attributes? If so, we should create an issue about that.

src/main/java/com/cburch/logisim/std/memory/Ram.java Outdated Show resolved Hide resolved
src/main/java/com/cburch/logisim/std/memory/Ram.java Outdated Show resolved Hide resolved
Comment on lines 615 to 616
ramInvertOutputEnable = Invert output enable
ramInvertWriteEnable = Invert write enable
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, it would be clearer for the users if we speak in case of enable, reset, and set signals of activity instead of negated/inverted inputs, i.e., the attribute refers to the control signal and the user has the choice between "high-active" and "low-active".

@figurantpp, @davidhutchens, @BFH-ktt1: What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like "Output Enable Mode: High Active/Low Active" sounds ok to me

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it is clearer to state the active level rather than speak of inverting something. Active High/Active Low sounds better to my ear than High Active/Low Active.

@maehne
Copy link
Member

maehne commented Sep 11, 2023

Note to @BFH-ktt1: This adds memory with inverted enables to the list of things that are not supported in hdl. That doesn't break anything that currently exists, but should the hdl generation be enhanced to cover these attributes? If so, we should create an issue about that.

IMHO, it would be nice if the activity is considered directly in the HDL generation. It should be fairly easy to add with the help of generic parameters, e.g., called OE_ACTIVE and WE_ACTIVE, which by default are '1'.

@davidhutchens
Copy link
Member

There are two other sets of enables with RAM. There are the Byte Enables (BE) and the Line Enables (LE). Those could also be Active High or Active Low. There are up to 8 of each. The BE and LE are never active together so only one additional Attribute would be necessary assuming it makes sense to force all 8 to have the same Active Level.

@maehne and @BFH-ktt1: Does what I wrote make sense? Remember, I am not a hardware person.

If it does make sense, should it be added to this PR or done later?

@cosineblast
Copy link
Contributor Author

cosineblast commented Sep 13, 2023

Note to @BFH-ktt1: This adds memory with inverted enables to the list of things that are not supported in hdl. That doesn't break anything that currently exists, but should the hdl generation be enhanced to cover these attributes? If so, we should create an issue about that.

IMHO, it would be nice if the activity is considered directly in the HDL generation. It should be fairly easy to add with the help of generic parameters, e.g., called OE_ACTIVE and WE_ACTIVE, which by default are '1'.

I'm not particularly used with HDL, but I can try working with it.
Edit: Yeah, that would take a little while for me. I believe it is better to open an issue and leave the HDL low-active oe part for a later time.

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

3 participants