icestudio icon indicating copy to clipboard operation
icestudio copied to clipboard

Hide or enable the inout feature so it only appears for "advanced" users?

Open TimRudy opened this issue 3 months ago • 15 comments

To appeal to different users (learners and others), I was thinking Icestudio could have an "Advanced" type of sub-menu in Preferences.

The new feature inout ports would be good for this, because who is going to want inout ports? For most people, they won't have to see the "InOut pin" (the 3rd check box) when creating/editing an Input or Output.

Do you want this? I can open a PR putting the "Include or Don't include" menu choice right in the Preferences menu. Or, as in the second screen shot, I could create an "Advanced" sub-menu with 2 or 3 items in it.

Please let me know which text you would choose, e.g.:

  'Include inout ports'
  'Include inout port type'
  'Advanced: inout ports'
  'Allow tri-state connections'
  'Include tri-state'

IncludeTristate1

P.S. I've thought about issues like this: "Normal" user opens a design that has inout ports in it. Icestudio would internally toggle to show inout ports, without further action. Most easily, this would mean: The user's profile would also update, storing the "Include inout ports" flag without further action. (They probably would not mind, and they could change it back afterward.)

This second menu idea could be a direction to go, what do you think? Name "Power user", or "Advanced...":

IncludeTristate2

TimRudy avatar Apr 03 '24 04:04 TimRudy

Hi Tim, thanks again for your feeback !!

I like a lot your proposal, I prefer the second choice but with "Advanced features" -> "Allow tri-state connections"

The choice of options for power users like a lot but i think "power users" could be confused for some people and could be newbie user need tri-state and naturally not identify it with "advanced users".

Do you like it? Go ahead with the PR!!!

cavearr avatar Apr 03 '24 05:04 cavearr

Thx - What other things should go in the Advanced features menu?

  • Remote hostname - used by anybody? Code says: //-- TODO: Think about removing this function in future versions
  • Python environment - is it needed in emergencies, trying to get Icestudio/Apio to work?
  • External plugins - would they be, mainly, for using Icestudio/Verilog/simulation in an advanced way? Or will plugins be for look & experience & cool features of Icestudio, for SigRok, for test benches, for debugging - then I'd say they're of potential interest to anyone

So if my assessment is OK, I would put in the Advanced menu:

  • Board rules (turning them off is quite unusual)
  • Allow tri-state connections
  • Remote hostname

TimRudy avatar Apr 03 '24 17:04 TimRudy

Hi @TimRudy ! for the moment if you want, put Python environment too in advanced tab , only is used if you have your own python installation in some non standard directory or outside the PATH environment.

External plugins will be a great feature for the next big update (very soon), i'm polishing plugin architecture to make easy that all of you could create new functionalities very easy, put in the advanced menu too.

Thanks again!

cavearr avatar Apr 03 '24 18:04 cavearr

Hi Carlos, Should I bump the project version

common.js: VERSION = “1.3” ?

Also another thing I thought of: There’s not a strong reason for this: But because of “inout” being an optional concept, I can take all the “inout”: false occurrences out of proj file at “prune” time. Only save “inout”: true occurrences.

On Apr 3, 2024, at 2:11 PM, Carlos Venegas Arrabé @.***> wrote:

 Hi @TimRudy ! for the moment if you want, put Python environment too in advanced tab , only is used if you have your own python installation in some non standard directory or outside the PATH environment.

External plugins will be a great feature for the next big update (very soon), i'm polishing plugin architecture to make easy that all of you could create new functionalities very easy, put in the advanced menu too.

Thanks again!

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

TimRudy avatar Apr 05 '24 18:04 TimRudy

Hi @TimRudy! At the moment we do not change the version of the project, I have planned it for the near future for other reasons, but at the moment it could be problematic to change it.

For this I want to adjust more things, version control within icestudio.

cavearr avatar Apr 05 '24 18:04 cavearr

Thinking about it, I just realized a problem.

If a user has this setting disabled and opens a design that requires inout ports, they should be able to do so, or they should be prompted to enable it.

The easiest thing would be to have three states for the inputs, with one more class it is simple.

We would have the normal state, the hidden state (they are not displayed) and the "disabled" state. What I would do is remove them disabled and with a degree of opacity, for example, 0.6 so that they can be seen but cannot even be changed and understood. visually it is somewhat disabled.

What do you think?

It is important that these new options are only for visualization and do not affect the project itself.

cavearr avatar Apr 05 '24 19:04 cavearr

I've been doing test cases - there are a lot. 2 examples so we can talk specifically:

  1. Existing project files accept "inout" appearing, when setting is disabled as above. One can do "Add block..." and the block contains "inout"s. Then Save and Save As will preserve the "inout"s in the model. I found that a bit surprising - but OK, fine.
  2. A crash can come along "later" when one has a project like case 1 open. E.g., the added block can have all "inout" ports on the right side. In that case the "out" array is found to be undefined here in editBasicCode, when opening the block: let outPortNames = blocks.portsInfo2Str(block.data.ports.out);

Scenario 2 is just code that can be fixed because it's not robust enough. However, considering case 1, I can understand why you are talking about the "disabled" visual state. It's because you know from experience that "inout" is not considered "Invalid project format". It should be "tolerated", it is kind of expected. Correct?

So: I don't think "easiest" is the word I'd use for having a third "disabled" visual state... :-) Is it good for the user? What is easiest for the user? If a "no-tri-state" user opens a project file or adds a block that has "inout" ports, how about this:

  • recognize this mismatch as early as possible
  • go with this plan:

they should be able to do so, or they should be prompted to enable it

  • So I can prompt "You are adding code that uses "tri-state". You can only do this if you accept the design consequences/complexity of this feature by updating your user preferences: "Advanced features -> Allow tri-state connections". Click ("Update Preferences") or ("Just while this file is open"), or ("Cancel")"
  • How about that? I just need to recognize as early as possible, by using a couple of locations in the code to handle all scenarios. And the flag for "Just runtime - do not save to profile.json", hopefully that doesn't have any edge cases.

TimRudy avatar Apr 07 '24 16:04 TimRudy

Hi @TimRudy ! give me the day to think in it please!

cavearr avatar Apr 08 '24 08:04 cavearr

I've been looking into it and let me know what you think about the following.

Regarding "tolerance", you're right, the model is not "strict," which leaves open situations like these that are relatively flexible, though not 100% robust.

I wanted to go over it because I'm making changes in this regard and didn't want to "step on toes." What do you think if I provide you with some functions that tell you if there are inouts in the design, so you could directly check during loading, and if it returns true, display the message and activate them?

About showing them or not, I agree with you, upon further reflection, it's better that the user doesn't see anything about inouts unless they activate it.

I think the idea of having a state to enable inouts only during execution is very good.

If you agree, I'll prepare this first version of the "linter" with that check, and you tell me how you see it.

cavearr avatar Apr 10 '24 06:04 cavearr

This is great. I don't want to step on toes, we can collaborate. On the weekend I tried injecting the checks these places:

    function pruneBlock(block) {                <-- now call it pruneAndScanBlock(block) {
      // Remove all unnecessary information for a dependency:
      // - version, board, FPGA I/O pins (->size if >1), virtual flag
      delete block.version;
      delete block.design.board;
      var i, pins;                                           <-- and var isInoutPorts = false;
      for (i in block.design.graph.blocks) {
        if (block.design.graph.blocks[i].type === blocks.BASIC_INPUT ||
          block.design.graph.blocks[i].type === blocks.BASIC_OUTPUT ||
              ...
          delete ...

          if (block.design.graph.blocks[i].data.inout) {         <-- a check (the only check?)
            isInoutPorts = true;
          }
        }
      }
      return block;                            <-- now return { block, isInoutPorts };
    }

There are 2 places where pruneAndScanBlock(block) is called. In the following location, this.addBlock, I think block is basically added to the model; therefore it might be a bit too late. I needed to see how to reverse the load - or maybe there is a better location to use:

block = _safeLoad(block);
let { block: newBlock, isInoutPorts } = pruneAndScanBlock(block);
if (isInoutPorts) doPrompt()
return early, or not

Thanks for taking a look at it

TimRudy avatar Apr 10 '24 12:04 TimRudy

Test Cases:

1. Regression tests:

  • Open, New, add & edit components, navigate in and out & unlock, edit, lock, Save, Save as
  • Bring in blocks using Add as block, Paste, drag from Collection Manager
  • Copy, Paste, Clone, Duplicate
  • Content fit is done at right time after opening designs, imports, navigate in and out, window resize
  • Loading animation is shown and dismissed at right times when action is taken & when transition happens

For next tests:

* Prepare an .ice file with "inout" port in it, can be Basic Input, Basic Output (this does not include "IceIO" collection devices, which declare the ports without using "inout") * Prepare an .ice file with a Code block in it, that in turn has "inout" port (can be Left or Right) * Place an .ice file with "inout" into a folder under user.../.icestudio/collections/<IceCollection>/blocks/ so it will appear in Collection Manager

* Note existing Icestudio behaviour is to accept data elements "inout", "inoutLeft", "inoutRight" in the existing file format. Even though Verify and Build may fail, those data elements are imported, copied around, saved transparently.

* This PR leaves the file format at version "1.2"

* In the following, "normal" file or block means => no "inout" data

User with "Allow tri-state connections" == false

Next 2 tests show:
Normal files & normal user's workflow will never trigger the "Tri-state"/"Advanced" warning

2. Start new version of Icestudio.

-> Profile at user.../.icestudio/profile.json picks up "allowInoutPorts: false"

3. Open normal file. Also New. Also Add as block (normal block). Do edits.

A) Add & edit port. B) Add & edit Code block. -> No fields are displayed for "inout" C) Save, and Save as. -> Get normal file (no "inout" data is written out) D) Similar tests, dragging a block from Collection Manager. Save, Save as. E) Similar tests, navigating into a block. Save, Save as. F) Similar tests using Paste, Duplicate, Clone. Save, Save as.

Next 2 tests show:
New prompt displayed for "inout"; normal user when they do not upgrade their profile

4. Open "inout" file. Also Add as block ("inout" block). Also drag "inout" block from collection in Collection Manager.

Always respond with "Cancel". Repeat after the cancel; cancel again. -> New prompt is displayed -> Nothing happens to the design -> Loading animation is dismissed if applicable -> Other functionality works as normal afterward -> Profile at .../profile.json still has "allowInoutPorts: false"

5. Open normal file. Do an edit. New to open a second window for Copy & Paste test.

Normal file opened is window #1.
In window #2, Open a block that has "inout". Respond "OK" and "This time" to the prompt.
(*i.e. no change to profile on disk)
Ctrl-C to Copy the component that has "inout". A) In window #1, Ctrl-V to Paste the "inout" block. -> "Tri-state connections" prompt is triggered in window #1 because both windows are "aware" independently B) "Cancel". Also repeat Paste & "Cancel". Save as. Quit window #1. Then Quit window #2. Also repeat, #2 then #1. -> On "Cancel", nothing happens to the design -> On Save as, get normal file (search "inout", no result) -> After any Quit, profile at .../profile.json still has "allowInoutPorts: false"
(*i.e. if never accept the "Tri-state connections"/"Advanced" Preferences change, never changes profile) C) During same test, in window #2, Ctrl-V, Ctrl-Shift-V to Paste, Paste Clone. -> There is no prompt, paste works fine D) During same test, open a new window #3, from window #2, from window #1. Paste. Also open a block that has "inout". -> There is a prompt; window #3 is independent -> After any Quit, profile at .../profile.json still has "allowInoutPorts: false"

Next test shows:
New prompt; responding to prompt in 2 open Icestudio windows, or after close & re-open Icestudio;
  when user does upgrade their profile;
  when user views "inout" only temporarily, then re-opens Icestudio

6. Open normal file. New to open a second window, as above.

Normal file opened is window #1.
In window #2, open a block that has "inout". Respond "OK" and "Yes".
(*i.e. a change to profile on disk) A) Quit window #2. Or Edit, Save then Quit window #2. Then Quit window #1. B) Quit window #1. Then Quit window #2. -> The windows still act independent (a window opened before the accept with "Yes" is not
"aware" of the new Advanced preference setting) -> The order of closing windows doesn't matter. The accept with "Yes" is saved, and it persists

   (* The behaviour is not perfect. Each window (ideally) would subscribe and get a
       notification to update its "inout" setting to the saved profile.
       This would remove "unnecessary" prompts. It would make the Preferences menu
       "Advanced features -> Allow tri-state connections" consistent between the 2 windows)
   (* Another behaviour: When prompt "Yes" is saved (profile is saved) in window #1,
       then there is a prompt in window #2 but the response is "This time", it changes the
       profile back, undoing what was "persisted" from window #1)
   - But the above are edge cases

User with "Allow tri-state connections" == true

Next test shows:
New Advanced mode for tri-state: Activating, persistence, the new fields during editing

7. Open "inout" file. Also Add as block. Also drag from collection in Collection Manager.

This time respond with "OK, not "Cancel". A) Respond "Yes" in 2nd prompt (upgrade to Advanced). -> Any further actions with "inout" blocks are normal, there is no prompt -> Profile at .../profile.json changes to "allowInoutPorts: true" B) Same test as (A). Quit Icestudio. Open normal file this time. Also New.
Add & edit Basic Input, Output. Add & edit Code block. -> Checkbox for "inout" is still displayed in port dialog -> Fields for left and right "inout" ports are still displayed in Code block dialog -> Profile at .../profile.json remains "allowInoutPorts: true" C) Respond "This time" in 2nd prompt. -> Any further actions with "inout" blocks are normal, there is no prompt -> Profile at .../profile.json still has "allowInoutPorts: false" D) Same test as (C). Quit Icestudio. After starting Icestudio, drag from collection again. -> Prompt is displayed again E) Same test as (C). Quit Icestudio. Open normal file this time. Also New.
Add & edit Basic Input, Output. Add & edit Code block. -> No checkbox is displayed in port dialog for "inout" -> No fields are displayed in Code block dialog for left or right "inout" ports -> No prompt is displayed

TimRudy avatar Apr 22 '24 04:04 TimRudy

Here is proposed text (when a user first sees a tri-state component/.ice file):

Would you translate it to Spanish and get a feeling for any changes/improvements you want?

You are loading a design that uses "tri-state". Tri-state (aka high-Z, bidirectional, or inout) ports are not recommended in standard designs.
You will be asked to update your Preferences (Advanced user setting) or you can just open this design on a preview basis.
Continue?

2nd version:

You are importing a block that uses "tri-state". ...<remainder the same>
Continue?

Then 2nd dialog:

Click "Yes" to allow tri-state and update Preferences:
   **Advanced features -> Allow tri-state connections**
Click "This time" to view tri-state for this design only.

TimRudy avatar Apr 22 '24 04:04 TimRudy

Hi @TimRudy ! i'm trying it this days and all works very well for the moment! thanks a lot of newbie folks gratefully you a lot!

cavearr avatar Apr 25 '24 15:04 cavearr

You're welcome! Message me if you want me to help on one of the tasks on your plate

TimRudy avatar Apr 25 '24 19:04 TimRudy

Thank for all @TimRudy , i'm publish my roadmap very soon, i hope that you join to some tasks if you want!

In other hand i'm thinking in use your 74s collection to do an interesting example like a 1-bit cpu or something like this in an hybrid environment between icestudio/web/fpga could be very interesting if we start a thread if you want and propose a collaborative work around it. Think in it!

cavearr avatar Apr 25 '24 21:04 cavearr