sylefeb / Silice

Silice is an easy-to-learn, powerful hardware description language, that simplifies designing hardware algorithms with parallelism and pipelines.
Other
1.28k stars 77 forks source link

General code issues thread #108

Open TaraHoleInIt opened 3 years ago

TaraHoleInIt commented 3 years ago

I'll start the first one:

In my attempt to debug WaitForRisingEdge by passing in a debug LED I now receive the following error message:

^^^^^^^^^^^^^^^^^^^ incorrect input parameters in call to algorithm 'WaitForRisingEdge', last correct match was parameter 'Signal'

I've checked the documentation and examples a few times, and its not immediately obvious why this is coming up.

$$Period = 1 / Frequency;
$$NSPerClock = 1000000000 * Period;
$$ClocksPerMS = math.floor( 1000000 / NSPerClock );

algorithm Debounce( input uint1 Source, output uint1 Dest ) <autorun> {
    uint24 Clocks = 0;
    uint8 Bitmap = 0;

    while ( 1 ) {
        Clocks = Clocks + 1;

        if ( Clocks == $ClocksPerMS$ ) {
            Bitmap = ( Bitmap << 1 ) | Source;
            Clocks = 0;
        }

        // If all the bits are set then the button should have finished bouncing
        // and the output should be reliable.
        Dest = ( Bitmap == 8hff ) ? 1 : 0;
    }
}

subroutine WaitForRisingEdge( input uint1 Signal, output uint1 Debug ) {
    uint1 Last = 0;

    Debug = 1;

    while ( 1 ) {
        if ( Last == 0 && Signal == 1 ) {
            break;
        }

        Last = Signal;
    }

    Debug = 0;
}

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {
    uint1 FilteredButton = 0;
    uint1 Button = 0;
    uint1 State = 0;

    Debounce Btn(
        Source <: Button,
        Dest :> FilteredButton
    );

    Button := gn[ 10,1 ];
    leds[ 0,1 ] := State;

    while ( 1 ) {
        // Error
        // ^^^^^^^^^^^^^^^^^^^ incorrect input parameters in call to algorithm 'WaitForRisingEdge', last correct match was parameter 'Signal'
        ( ) <- WaitForRisingEdge <- ( FilteredButton, LED );
        State = ~State;
    }
}
sylefeb commented 3 years ago

The syntax for the subroutine call is: (output0,...,outputN) <- sub <- (input0,....,inputK) So the line has to be changed for (LED) <- WaitForRisingEdge <- ( FilteredButton)

Something else, more related to timing -- in this part:

   Clocks = Clocks + 1;
   if ( Clocks == $ClocksPerMS$ ) {
    // ...
   }

It is best to increment Clock after the if (adjusting the logic accordingly). This will help timing as the test and addition will no longer depend on each other within the same cycle. (I started writing guidelines for this, hoping to finish soon). In this case this will have little impact, but it is a good habit to take.

(Edit:) the same is true for Dest = ( Bitmap == 8hff ) ? 1 : 0; which is used in the condition after being updated in the if. Btw, this could also be written with an always assign, writing Dest := ( Bitmap == 8hff ) ? 1 : 0; above the while(1) (this will effectively assign Dest first thing every clock cycle). In this case it does not really impact, but that can be very convenient if Dest has to be constantly updated regardless of the state of the algorithm.

TaraHoleInIt commented 3 years ago

Thank you for the feedback! I will keep this in mind for the future.

That being said, I'm still having trouble getting WaitForRisingEdge to work:

subroutine WaitForRisingEdge( input uint1 Signal ) {
    uint1 Exit = 0;
    uint1 Last = 0;

    Last = Signal;

    while ( Exit == 0 ) {
        Exit = ( Last == 0 && Signal == 1 ) ? 1 : 0;
        Last = Signal;
    }
}

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {
    uint1 FilteredButton = 0;
    uint1 Button = 0;
    uint1 State = 0;
    uint1 ADebug = 1;

    Debounce Btn(
        Source <: Button,
        Dest :> FilteredButton
    );

    Button := gn[ 10,1 ];
    leds[ 0,1 ] := ADebug;

    gp[ 0,1 ] := State;

    while ( 1 ) {
        ADebug = 1;
            ( ) <- WaitForRisingEdge <- ( FilteredButton );
        ADebug = 0;

        State = ~State;
    }
}

I'm not sure if its a code problem or FPGAs being a bit unfamiliar. As far as I can tell it never actually returns, but FilteredButton is behaving just as I would expect.

sylefeb commented 3 years ago

I think I see the problem. When you call a subroutine it copies its arguments - precisely to avoid them changing while it processes. This is why WaitForRisingEdge does not see FilteredButton change.

Two possible approaches. 1) Incorporate the subroutine in the algorithm (which they are meant to be), so you can actually access the algorithm internals. This becomes:

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {

    uint1 FilteredButton = 0;
    uint1 Button = 0;
    uint1 State = 0;
    uint1 ADebug = 1;

        subroutine WaitForRisingEdge( reads FilteredButton ) {
            uint1 Exit = 0;
            uint1 Last = 0;

            Last = FilteredButton ;

            while ( Exit == 0 ) {
                Exit = ( Last == 0 && FilteredButton == 1 ) ? 1 : 0;
                Last = FilteredButton;
            }
        }

    Debounce Btn(
        Source <: Button,
        Dest :> FilteredButton
    );

    Button := gn[ 10,1 ];
    leds[ 0,1 ] := ADebug;

    gp[ 0,1 ] := State;

    while ( 1 ) {
        ADebug = 1;
            ( ) <- WaitForRisingEdge <- ( );
        ADebug = 0;

        State = ~State;
    }
}

Note the explicit declaration reads FilteredButton ; this is to make it clear what a subroutine is supposed to manipulate.

The other option is to turn WaitForRisingEdge into another algorithm and use parameter bindings:

algorithm WaitForRisingEdge( input uint1 FilteredButton ) {
  uint1 Exit = 0;
  uint1 Last = 0;

  Last = FilteredButton ;

  while ( Exit == 0 ) {
    Exit = ( Last == 0 && FilteredButton == 1 ) ? 1 : 0;
    Last = FilteredButton;
  }
}

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {

    uint1 FilteredButton = 0;
    uint1 Button = 0;
    uint1 State = 0;
    uint1 ADebug = 1;

    Debounce Btn(
        Source <: Button,
        Dest :> FilteredButton
    );

        WaitForRisingEdge waitedge(
            FilteredButton <: FilteredButton, // parameter binding
        );

    Button := gn[ 10,1 ];
    leds[ 0,1 ] := ADebug;

    gp[ 0,1 ] := State;

    while ( 1 ) {
        ADebug = 1;
        () <- waitedge <- (); // no input parameter as it is bound
        ADebug = 0;

        State = ~State;
    }
}

When a parameter is bound it tracks the value of the source (connected through a wire in terms of hardware). Note that there are two bindings: <: tracks the value as it is updated during the cycle, <:: tracks the value at it was at the cycle starts (this is a topic I will document better, it is a very interesting degree of freedom to play with in designs). In short, <: is typical and allows the algorithm to immediately start refreshing the outputs when a value is changed -- but this can also reduce the max frequency of the design as the signal propagation deepens. <:: will trigger a refresh only at the next cycle: pay one in latency but increase max freq.

TaraHoleInIt commented 3 years ago

I have done some testing with generating delays, it seems like ones done inline are accurate while wrapping them in a subroutine causes problems.

For example, if I have a counter in my main loop and toggle a pin whenever the counter reaches 4 then I get a 250ns signal which is what I would expect from a 16MHz clock. Trying to make this neater by wrapping it up in a subroutine is nowhere near as accurate.

Maybe I'm just approaching this wrong and there is a more correct way to accurately handle timing?

sylefeb commented 3 years ago

Hi - the timing would have to account for the subroutine entry/exit cycles. This is partially discussed in Section 5 but I really have to revise this with more examples and diagrams, that is an important topic.

So for instance, let's consider this wait subroutine in a small test case:

subroutine wait(input uint16 incount)
{
  uint16 count = uninitialized;
  count = incount;
  while (count != 0) { count = count - 1; }
}

algorithm main( output uint$NUM_LEDS$ leds,input uint1 btns ) 
{
  uint16 cycle = 0;

  always { cycle = cycle + 1; }

  __display("[before] cycle %d",cycle);
  () <- wait <- (0);
  __display("[next  ] cycle %d",cycle);
  () <- wait <- (4);
  __display("[done  ] cycle %d",cycle);
}

Now simulating with icarus make icarus (Makefile at the end) we get:

[before] cycle     0
[next  ] cycle     4
[done  ] cycle    12

So we can see the subroutine call, even with a 0 parameter takes 4 cycles. With an input of 4 it then takes 8 cycles. So there is an overhead of four: 2 for entry/exit of subroutine and 2 for the entry/exit of the while. Note that there are chain rules, so adding a while right after the other will add a single cycle instead of 2 (clarifying the documentation right now!).

What if we want a less than 4 cycles wait? We can use multiple ++: operators, or use a circuitry to inline a while loop with a parameter for its duration.

Taking these delays into account should give you the exact timing.

.DEFAULT: test.ice
        silice-make.py -s test.ice -b $@ -p basic -o BUILD_$(subst :,_,$@)

clean:
    rm -rf BUILD_*
TaraHoleInIt commented 3 years ago

Ohhhh okay! I think I see what was happening then.

I don't think I was taking into account not only that entering and exiting the subroutine were taking cycles, but that the main loop was adding some delay as well. That being said, nothing I'm doing now requires timing that precise, I just wanted to understand the behaviour I was seeing.

Thank you :)

at91rm9200 commented 2 years ago

Hello,

I am a Silice and FPGA newbie and have a question regarding readwrite permissions on subroutines.

In the following example the subroutine out_led is called by the subroutine out_led_inverted. For this, the subroutine out_led_inverted needs to get permission to access the parameter pattern from out_led.

algorithm main(output uint5 leds) {

uint26 cnt = 0;

   subroutine out_led(input uint5 pattern, readwrites leds) {
      leds = pattern;
   }

   subroutine out_led_inverted(input uint5 pattern_inv, readwrites i_out_led_pattern) {
   //                                                              ^^^^^^^^^^^^^^^^^
      () <- out_led <- (~pattern_inv);
   }

   while(1) {
      cnt = cnt + 1;
      () <- out_led_inverted <- (cnt[21,5]);
   }
} 

But the syntax for this is not clear to me. Only from the compiler error message "variable 'i_out_led_pattern' is written by subroutine 'out_led_inverted' without explicit permission" I could recognize the name for this parameter. Is this intentional or did I miss something?

Regards, Bernd.

sylefeb commented 2 years ago

Hello Bernd,

The error message is indeed not useful, I'll improve that. The subroutine out_led_inverted has to explicitly declare it intends to call out_led. Here is how to do that: https://github.com/sylefeb/Silice/blob/5fa9ed995c20f7bc9654e7c63a858844efb26fbc/tests/issues/108_rw_perm.ice#L9

Welcome to Silice! Best regards, Sylvain

sylefeb commented 2 years ago

Improved error message (in wip branch):

() <- out_led <- (~pattern)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ subroutine 'out_led_inverted' calls other subroutine 'out_led' without permssion
                            add 'calls out_led' to declaration if that was intended.

error: Silice compiler stopped

Thanks for pointing this out!!

at91rm9200 commented 2 years ago

Hello Sylvain,

thank you for the explanation. It works perfectly.

Regards, Bernd.

at91rm9200 commented 2 years ago

Hello,

I am trying to bind two pmod bits, one as input and the other one as output, to an algorithm. The input binding "bit_in <: pmod.i[0,1]" seems to work, but the output binding "bit_out :> pmod.o[1,1]" leads to an error: "cannot find accessed base.member 'pmod.o'". For sure, this is a beginner question. But I cannot see, how to solve this.

algorithm alg_test(input uint1 bit_in, output uint1 bit_out)
{
   bit_out = ~bit_in;
}

algorithm main(output uint5 leds, inout uint8 pmod) {

   alg_test a(bit_in <: pmod.i[0,1], bit_out :> pmod.o[1,1]);  // <-- cannot find accessed....

   while (1) {
      pmod.oenable = 8b00000010;
      () <- a <- ();
   }
}  

Regards, Bernd.

sylefeb commented 2 years ago

Hi Bernd,

Your syntax makes perfect sense, unfortunately you've hit a limitation on output bindings (see #208). The error message is not helpful and I'll improve that shortly (ultimately the syntax your are using will be supported).

The good news is, there are simple workarounds, see https://github.com/sylefeb/Silice/blob/draft/tests/issues/108_pmod.ice

Thanks for the report! Sylvain

sylefeb commented 2 years ago

Partially fixed the issue:

Changes in draft branch.

at91rm9200 commented 2 years ago

Hello Sylvain,

thank you for the answer, for improving the Silice compiler and providing the workaround. I think, I am going to stick with the master branch for now.

Regards, Bernd.

at91rm9200 commented 2 years ago

Hello Sylvain,

can an inout pmod simply be used as a gpio? The following program should output four pulses on the icestick pmod (bit 0) and then read bit 1 (pulled up via a resistor) and output it on the LED. The program only works if the marked step is commented out. If the step is not commented out, then bit 1 is apparently not read in correctly. I suspect I am missing something significant, or may be the inout pmods can not be used this way?

algorithm main(output uint5 leds, inout uint8 pmod) {
   uint3 ctr = 4;
   uint1 pmod_bit1 = uninitialized;

   pmod.oenable:= 8b00000001;
   pmod.o = 8b00000001;

   while (ctr > 0) {
      ctr = ctr - 1;
      ++:                       // <--- works, if this step is commented out.
      pmod.oenable = 8b00000001;
      pmod.o = 8b00000001;
      ++:
      pmod.oenable = 8b00000001;
      pmod.o = 8b00000000;
   }
   ++:
   pmod.oenable = 8b00000001;
   pmod_bit1 = pmod.i[1,1];     // pmod bit 1 is pulled high via resistor.

   leds[0,1] = pmod_bit1;

   while(1) {
   }
}  

Regards, Bernd.

sylefeb commented 2 years ago

Hi Bernd - thanks for the report, I am looking into it! (I see the same behavior and it is indeed confusing).

sylefeb commented 2 years ago

I believe I have identified the issue -- it's not obvious, and I am actually surprised by the behavior. Nevertheless I decided to modify the tristate so that the behavior is stable. This means introducing a one cycle latency between when .o / .oenable are set and the actual change on the pin, but that is anyway better in terms of managing IO skew/delays.

The change is applied in the wip branch ; I will soon merge with master but a few adaptations remain to be done in other projects. Please let me know if you have any trouble.

(Side note: instead of while (ctr > 0) use while (ctr != 0) to reduce LUT usage ;) An even better way is to make ctr signed and test whether the sign bit is set (meaning -1 is reached), adjusting the counter accordingly).

at91rm9200 commented 2 years ago

Hello Sylvain,

thanks for tracking this down. I think I will wait for the master branch. But I have one more question about the above example: The I/O configuration of the pmod does not change in the algorithm. It is always 8b00000001, seven inputs and one output in the same place. Shouldn't a single always assignment (pmod.oenable:= 8b00000001) be enough? Or does pmod.oenable have to be explicitly set before each pmod.o and pmod.i statement, as I did above?

Regards, Bernd.

sylefeb commented 2 years ago

Hi Bernd, the single assignment should indeed be enough (and is enough after the fix), the best being a single pmod.oenable := 8b00000001; since the := will hold the value at all times (unless overridden by the algorithm), allowing for a simpler logic. But even a single pmod.oenable = 8b00000001; at the top (without the :=) would do now.

The fact you needed multiple assignments was a side effect of the problem itself.

at91rm9200 commented 2 years ago

Hello Sylvain,

thank you for the explanation. I have come across the strange "pmod behavior" several times. I'm glad you were able to fix it.

Regards, Bernd.

sylefeb commented 2 years ago

Thanks a lot Bernd for the tests + reports (inouts are used in fewer designs, and tristates can be a bit tricky...). In general feel free to reach out if anything seems suspicious, I am happy to check.

at91rm9200 commented 1 year ago

Hello Sylvain,

I am having difficulties to get the dual ported BRAM working. The following design for the Icestick should use ICESTORM_RAM, but if I look into the log-file (next.log) I can see a utilization of 0 % for the RAM. I have the "simple_dualport_bram" working in another design, but this time I want to read from both ports.

unit main(output uint5 leds) {

   dualport_bram uint8 mem[128] = {0,1,2,3,4,5,6,7,8,9,pad(uninitialized)};
   uint7 ctr = 0;

   algorithm {

      while(1) {
         mem.wenable0 = 0;       // read port 0
         mem.addr0 = ctr;
         ++:
         leds = mem.rdata0;

         mem.wenable1 = 0;       // read port 1
         mem.addr1 = ctr + 1;
         ++:
         leds = mem.rdata1;

         ctr = ctr + 1;
      }
   }
}
Info: Device utilisation:
Info:            ICESTORM_LC:   170/ 1280    13%
Info:           ICESTORM_RAM:     0/   16     0%
Info:                  SB_IO:     6/  112     5%
Info:                  SB_GB:     3/    8    37%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%

I am not using the very latest version of yosys (0.20 ), nextpnr (0.3) and Silice.

It would be nice if you could check if I did something wrong.

Regards, Bernd.

sylefeb commented 1 year ago

Hi Bernd,

Just had a quick look, this is indeed odd. As a workaround comment mem.wenable0 = 0; and it will synthesize using brams again (tested on Yosys 0.17+76). As long as you only read on port 0 this is fine because the default (on fpga) will be 0 anyway. That actually means using the bram as a simple_dualport_bram that allows read/writes on port 1 and only reads on port 0.

I'll investigate further. Thanks for the report!

at91rm9200 commented 1 year ago

Hello Sylvain,

thank you. This works. I can read now over port 0 and read/write over port 1. As far as I can see, using dualport_bram (vs. simple_dualport_bram) seems to double the resources in the FPGA concerning ICESTORM_RAM. This would halve the amount of BRAM. However.

Bonne année.

Regards, Bernd.

sylefeb commented 1 year ago

Hi Bernd, interesting, I'll have a closer look at how Yosys synthesizes the BRAM. From the documentation it is unclear whether a 'true' dualport_bram is supported. I'll investigate and adjust the framework (and/or Verilog template) accordingly.

Best wishes for 2023! Sylvain

at91rm9200 commented 1 year ago

Hello Sylvain,

on the Gisselquist (zipcpu) blog, I found an article, which describes that the ICE40 does not contain dual ported BRAM with two separate read ports. As a "workaround" yosys uses two BRAMs with connected write ports.

https://zipcpu.com/dsp/2020/02/07/bad-histogram.html

This would at least explain the increase of ICESTORM_RAM.

Regards, Bernd.

sylefeb commented 1 year ago

Hi Bernd, great finding. It makes sense as yosys chooses to write the same to both and then read separately (I use a similar trick for registers in my riscv CPUs, but doing it 'manually' with two BRAMs). This can be seen in the build.json file, searching for "SB_RAM40_4K" you'll find two instances "__main.__mem__mem.buffer.1.0" and "__main.__mem__mem.buffer.0.0". Both are indeed receiving the same write signals (eg. "WADDR": [ 94, 95, 96, 97, 98, "0", "0", "0", 99, 100, "0" ]). Always amazed by yosys inference capabilities! So I think I will disable dualport_bram for the ice40s: the compiler will stop on an unsupported error, suggesting to use a simple_dualport_bram or registers instead -- I find this less confusing and more true to the hardware capability.

at91rm9200 commented 1 year ago

Hello Sylvain,

in my case, the current Silice/yosys behavior solves my problem. Even if the RAM utilization doubles. As a FPGA-novice (using Silice exclusively since about one year), I would have a lot of trouble, to handle the BRAM manually, as you did with your RISC-V cpu.

Maybe you could introduce something like a restricted_dualport_bram for the ICE40 and give a hint in the documentation that this variant provides one read/write and one read port and is built out of two BRAMS.

Regards, Bernd.

at91rm9200 commented 1 year ago

Hello Sylvain,

Can an algorithm that was called asynchronously be called a second time (asynchronously) without being finished the first time?

For example, I have this timer unit:

unit timer24(input uint24 value, output! uint1 timeout)
{

   uint24 local_value = uninitialized;

   algorithm {
      timeout = 0;
      local_value = value;

      while (local_value != 0) {
         local_value = local_value - 1;

      }
      timeout = 1;
   }
}

I want to monitor a timeout as follows:

unit main()
{
   uint1 timer1_timeout = uninitialized;
   timer24 timer1(timeout :> timer1_timeout);

   algorithm
   {
      while (1) {
         timer1 <- (100000);
         ++:
         while(check_for_event1) {
            if (timer1.timeout == 1) {
               goto exit_error;
            }
         }

         // the timer is not expired here. The timer algorithm is still running.
         timer1 <- (100000); // What happens here?
         ++
         while(check_for_event2) {
            if (timer1.timeout == 1) {
               goto exit_error;
            }
         }

         // ..... normal processing

      }
      exit_error:
   }
}  

Can it be done this way? I doubt it.

Regards, Bernd.

sylefeb commented 1 year ago

Hi Bernd,

If an algorithm is called while already running, it will be forced to restart:

algorithm test()
{
  uint8 i=0;
  while (i<12) {
    __display("i = %d",i);
    i = i + 1;
  }
}

algorithm main(output uint8 leds)
{
  test t1;
  t1 <- ();
++: ++: ++: ++:
  t1 <- (); 
  while (!isdone(t1)) { }
}

Output is:

i =   0
i =   1
i =   2
i =   0 // forced restart
i =   1
i =   2
i =   3
i =   4
i =   5
i =   6
i =   7
i =   8
i =   9
i =  10
i =  11

If I understand correctly that would fit your use case? Another situation this is useful is when the algorithm first state contains a pipeline to be fed (feature still being documented)

Otherwise the algorithm has to be instanced multiple times (as many as the number that has to run in parallel).

at91rm9200 commented 1 year ago

Hello Sylvain,

Thank you for the explanation. The behavior fits my use case quite exactly. I want to run a single instance of an algorithm multiple times without waiting for it to finish. If the algorithm is restarted when it is called again, that's perfect.

Regards, Bernd.

at91rm9200 commented 1 year ago

Hello Sylvain,

the draft version behaves differently than the master version regarding local variables in subroutines. The master version compiles the following design without problems, the draft version throws an error in line 9:

unit main(output uint5 leds, inout uint8 pmod)
{
   uint3 ctr = 0;

   algorithm
   {
      subroutine delay(input uint24 cycle_counter)
      {
         uint24 ctr = 0;   // <----- Line 9: variable 'ctr': this name is already used by a prior declaration

         ctr = cycle_counter;
         while (ctr > 0) {
            ctr = ctr - 1;
         }
      }

      while (1) {
         leds = leds + 1;
         ctr = ctr + 1;
         () <- delay <- (2000000);
         if (ctr == 0) {
            leds = 0;
         }
      }
   }

}

Is this a problem of the draft version?

Regards, Bernd.

sylefeb commented 1 year ago

Hi Bernd - thanks for the report. Indeed this changed in draft but has to be revised. In general it is best to avoid shadowing a variable name, but I intend to issue a 'strong' warning as opposed to an error on such cases (and there are other similar cases that needs clarifying too). Sorry for the trouble, will try to look into it soon! (Opened new issue #250)

sylefeb commented 1 year ago

Hi Bernd, this problem (name shadowing) should be fixed in master, please let me know in case of trouble.

at91rm9200 commented 8 months ago

Hello Sylvain,

I am trying to realize an 8 bit bidirectional data transfer with an ICE40 FPGA. The FPGA should read the external data with the falling edge of the clock. I found the module projects/common/ice40_sb_io_inout.v, which uses the SB_IO primitive to get the desired pin functions. Analog to the project spiflash2x.si, I tried to create the pin bindings without success:

import('ice40_sb_io_inout.v')

unit main(output uint5 leds,
          inout uint8 sdr_dq)

{
   uint8 io_oe(0); uint8 io_i(0); uint8 io_o(0);

   sb_io_inout sb_io0(clock <: clock,
                      oe <: io_oe[0,1],
                      in :> io_i[0,1],
                      out <: io_o[0,1],
                      pin <:> sdr_dq[0,1]);
                      ^^^^^^^^^^^^^^^^^^^ expecting an identifier on the right side of a group binding

   algorithm {
      while (1) {
      }
   }
}         

Obviously I am doing something wrong. Regards, Bernd.

sylefeb commented 8 months ago

Hi Bernd,

Sorry for the delay in answering. There is a limitation on tristate pins, they cannot be bound with a bit select (so the issue is sdr_dq[0,1]). Instead you either have to:

This is not great, I'll look into fixing this limitation! (opened issue #258). The error message also needs clarification.

Thanks and best wishes for 2024! Sylvain

at91rm9200 commented 8 months ago

ok, I am going to take the breakout route then. So I can avoid Verilog.

Have a good new Year. Bernd.

sylefeb commented 7 months ago

Hi Bernd, I have lifted this restriction (in draft branch), so that binding inouts with bit select is now possible. Please let me known if there's any trouble with it. Best, Sylvain

at91rm9200 commented 7 months ago

Hello Sylvain,

I am pleased that you have removed the bit select restriction and would like to try out the new version.

But I have problems to compile the draft version of Silice. The master version compiles without problems. I am still using Ubuntu 18.04 with gcc version 9.4.0 and cmake 3.22.1. If you say its too old, then I am going to update my build system. Otherwise:

cmake throws a lot of warnings like:

-- Detecting CXX compile features - done
CMake Deprecation Warning at antlr/antlr4-cpp-runtime-4.7.2-source/CMakeLists.txt:2 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

And later the build fails with:

[ 36%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/ParserInterpreter.cpp.o
[ 36%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/ParserRuleContext.cpp.o
[ 36%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/ProxyErrorListener.cpp.o
[ 37%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/RecognitionException.cpp.o
[ 37%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/Recognizer.cpp.o
In file included from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/PredictionContext.h:10,
                 from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/ATNSimulator.h:11,
                 from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/Recognizer.cpp:12:
/home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/ATNState.h:73:26: warning: type attributes ignored after type is already defined [-Wattributes]
   73 |   class ANTLR4CPP_PUBLIC ATN;
      |                          ^~~
[ 38%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/RuleContext.cpp.o
In file included from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/RuleContext.cpp:10:
/home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/ATNState.h:73:26: warning: type attributes ignored after type is already defined [-Wattributes]
   73 |   class ANTLR4CPP_PUBLIC ATN;
      |                          ^~~
[ 38%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/RuleContextWithAltNum.cpp.o
cc1plus: all warnings being treated as errors
CMakeFiles/libsilice.dir/build.make:115: recipe for target 'CMakeFiles/libsilice.dir/src/LuaPreProcessor.cpp.o' failed
make[2]: *** [CMakeFiles/libsilice.dir/src/LuaPreProcessor.cpp.o] Error 1
CMakeFiles/Makefile2:169: recipe for target 'CMakeFiles/libsilice.dir/all' failed
make[1]: *** [CMakeFiles/libsilice.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

Regards, Bernd.

at91rm9200 commented 7 months ago

Hello Sylvain,

I used the CMakeLists.txt from the master branch and could compile the draft branch with it. Now I have to upgrade Python, since Edalize seems to need a newer version...

Regards, Bernd.

sylefeb commented 7 months ago

Hi Bernd, thanks! I'll be looking into this shortly (this is due to the compiler treating warnings as errors, I thought I fixed it but clearly the change did not propagate).

at91rm9200 commented 7 months ago

Hello Sylvain,

I am now using the new bit select "feature" to bind the inouts and my design is working. Thank you very much for implementing this.

May be there is still a little problem, since Silice throws a lot of warnings that "bindings have inconsistent bit-widths", although the bit length is clearly one bit. Shall I open a new issue for this?

Then I had to use the silice-make.py from the master branch. Otherwise I got the following error:

<<=- compiling blinky.si for icestick -=>>
using default variant  configurable
using variant          configurable
using build system     edalize
Traceback (most recent call last):
  File "/home/bernd/fpga/silice/2401draft_weg/Silice/bin/silice-make.py", line 274, in <module>
    from edalize.edatool import get_edatool
ImportError: cannot import name 'get_edatool'
Makefile:3: recipe for target 'icestick' failed
make: *** [icestick] Error 1

And at last, I had to use the "dynamic linked variant" of Silice, which is produced with the master version of CMakeLists.txt. Executing the "static variant" leads to a segmentation fault on my machine.

Regards, Bernd.

sylefeb commented 7 months ago

Hi Bernd,

Thanks for the report and for having gone through all the trouble, I'm working on fixing all these issues asap!

Best, Sylvain

sylefeb commented 7 months ago

Hi Bernd,

All of these issues are fixed in the draft branch. Regarding compilation, I created a 'getting started' script ./get_started_linux.sh that I tested under Ubuntu (18, 20), Debian, Fedora, ArchLinux. Please note however that everything is now installed in the usual /usr/local/bin and /usr/local/share/silice directories, and the script also downloads and sets up an FPGA toolchain (oss-cad-suite, gets installed into /usr/local/share/silice/oss-cad-suite). It does add a line to .bashrc for setting up the environment. Of course you can still choose to compile Silice as before, only the installation paths are changed.

To avoid the old silice executable to be in the path it may be best to remove it from Silice/bin.

The warning is fixed too.

Hope that these changes will go smoothly! Best, Sylvain

at91rm9200 commented 7 months ago

Hello Sylvain,

I compiled the old way via compile_silice_linux.sh. This went smooth. Thanks.

/ Rant about Python removed ;-) /

There are still warnings left. Silice code:

   uint8 io_oe(0); uint8 io_i(0); uint8 io_o(0);
   sb_io_inout sb_io0(clock <: clock, oe <: io_oe[0,1], in :> io_i[0,1], out <: io_o[0,1], pin <:> sdr_dq[0,1]);
[warning]    (/home/bernd/fpga/silice/2402draft/Silice/mue/project/test_sdram/sdram_8.si, line   67) 
             instance 'sb_io0', binding 'clock' to 'clock', bindings have inconsistent bit-widths

I can provide a full test case, if you need it.

Regards, Bernd.

HusseinSam commented 2 months ago

Hello sylefeb,

I'm using Linux and starting to use Silice. In my first step, I tried to test it as you instructed:

cd projects cd divstd_bare make icarus

However, it shows me an error that it can't recognize the 'silice' command." , should i do something else after i cloned the repository and run ./get_started_linux.sh ? , its does not clear for me. regards, Hussein.

sylefeb commented 2 months ago

Hello, did you run compile_silice_linux.sh ? It is responsible for compiling and installing Silice (see also GetStarted_Linux). As the compilation script does a make install, the script will request sudo access. (edit: note on sudo)

HusseinSam commented 2 months ago

no i did not run "compile_silice_linux.sh" , because its already included in ./get_started_linux.sh command as you can see in the picture : Screenshot 2024-07-10 155031

and i got this output also on my terminal ( which is from compile_silice_linux.sh file ) : so its ok ? Screenshot 2024-07-10 155302

and another question should i do this by my self ?

Please compile and install:

sylefeb commented 2 months ago

These last steps should no longer be necessary if using the get_started_linux.sh command as it installs pre-built binaries from oss-cad-suite (I'll update this message). Can you check the content of Silice/BUILD/build-silice? There should be a silice executable in there. Were there any errors reporting when executing the script? (feel free to attach the full console log).

HusseinSam commented 2 months ago

here is the output : no error but there are many warnnings like this : image