6 ms·

The code below doesn't sound right, can you double-check the intent? I don't think timeout will go to 0, unless you make it a blocking assignment. if(~

The code below doesn't sound right, can you double-check the intent? I don't think timeout will go to 0, unless you make it a blocking assignment. if(~old_download && ioctl_download) begin cnt <= 8'b0; timeout <= 32'b0; end timeout <= timeout + 1'b1; That's why in Verilog RTL we generally try to keep always blocks as small as possible, so they focus on assigning the same small set of signals in all branches.

hamiltonkibbe 8y ago
I think that will simulate and synthesize correctly, but it does look a bit like code smell. If timeout is in a sensitivity list somewhere though, I'm not sure what will happen.
hrvach 8y ago
You are right, I missed that. Thanks for pointing it out! It's hard to change your mindset from a programming language to a hardware description language and not make mistakes like these, especially when just starting to learn Verilog. :-)
- alain94040 8y ago
  One way to restructure your code is to move all the default assignments to the top of the always block, and then let if conditions change the ones that don't apply for that cycle. This code would work just fine (many people would argue it's not very readable -- but it saves you a lot of else branches): // Default timeout <= timeout + 1'b1; // Special cases if(~old_download && ioctl_download) begin cnt <= 8'b0; timeout <= 32'b0; end
  - hrvach 8y ago
    Thank You for the advice! I'm still very much a beginner and since I find the "blink a led" kind of projects too boring, this is basically the first thing I ever wrote in Verilog. I hope my next project will have cleaner code, be more readable and have less issues like these. HDL languages seem to have a rather steep learning curve and it takes some effort to distance yourself from the usual programming mindset.
    - Taniwha 8y ago
      In general you should only use a <= to write a reg once within a clock (remember that the RHS of a <= is evaluated when the code is evaluated, the assignment happens later) And you are mixing <= and = in the same code (look in execute() ) this should never happen. In general use <= in places that are gated by an "always @(posedge clk)" and = in things gated by "always @(*)" (with the exception that it's OK to assign a temp variable in a clocked always with '=' provided its lifetime doesn't extend past the block's execution (your use of SKIP_FLAG is an example of this being done correctly)
      - hrvach 8y ago
        Thank you, I'll have to do some refactoring to avoid mixing the two assignment principles. But it is ok to do something like: cnt <= cnt + 1; if (cnt > 100) cnt <= 0; If not, how else should something like this be done? Thanks for your help, I'm finding HDL to be anything but easy. :-)
        Taniwha 8y ago
        probably not a good idea, because you are sampling cnt after think you have incremented it and you will get the wrong value Essentially what happens is: tmp_cnt1 = cnt + 1; if (cnt > 100) tmp_cnt2 = 0; some time later: cnt = tmp_cnt1; and after that (if cnt >100) cnt = tmp_cnt2; Better to say: if (cnt >= 99) { cnt <= 0; } else { cnt <= cnt+1; }
        hrvach 8y ago
        Thanks for pointing it out, I'll try to stick to a simple if-else construct in the future!
        FullyFunctional 8y ago
        There is certainly no consensus that it's the preferred style. Some designers prefer this explicit (and more verbose) style but others prefer the less verbose one (with default first, business logic next, and reset values at the end).
- tasty_freeze 8y ago
  Here is another suggestion that is part stylistic, but may save you debugging effort. When performing boolean condition testing, eg "if (~a & b)", always use "!" for negation instead of "~", "&&" for logical and, and "||" for logical or for exactly the same reasons you should do it for C code. If the operands are all 1 bit, either option works fine. But if you ever accidentally use a vector where you intended a bit, one generates a warning and the other doesn't.
jhallenworld 8y ago
I made a beginners guide to Verilog always blocks and assignment statements that talks about this very thing: https://www.youtube.com/watch?v=0pyQ0ER9Rqc https://www.youtube.com/watch?v=0pyQ0ER9Rqc

hamiltonkibbe 8y ago

I think that will simulate and synthesize correctly, but it does look a bit like code smell. If timeout is in a sensitivity list somewhere though, I'm not sure what will happen.

hrvach 8y ago

You are right, I missed that. Thanks for pointing it out! It's hard to change your mindset from a programming language to a hardware description language and not make mistakes like these, especially when just starting to learn Verilog. :-)

alain94040 8y ago
One way to restructure your code is to move all the default assignments to the top of the always block, and then let if conditions change the ones that don't apply for that cycle. This code would work just fine (many people would argue it's not very readable -- but it saves you a lot of else branches): // Default timeout <= timeout + 1'b1; // Special cases if(~old_download && ioctl_download) begin cnt <= 8'b0; timeout <= 32'b0; end
- hrvach 8y ago
  Thank You for the advice! I'm still very much a beginner and since I find the "blink a led" kind of projects too boring, this is basically the first thing I ever wrote in Verilog. I hope my next project will have cleaner code, be more readable and have less issues like these. HDL languages seem to have a rather steep learning curve and it takes some effort to distance yourself from the usual programming mindset.
  - Taniwha 8y ago
    In general you should only use a <= to write a reg once within a clock (remember that the RHS of a <= is evaluated when the code is evaluated, the assignment happens later) And you are mixing <= and = in the same code (look in execute() ) this should never happen. In general use <= in places that are gated by an "always @(posedge clk)" and = in things gated by "always @(*)" (with the exception that it's OK to assign a temp variable in a clocked always with '=' provided its lifetime doesn't extend past the block's execution (your use of SKIP_FLAG is an example of this being done correctly)
    - hrvach 8y ago
      Thank you, I'll have to do some refactoring to avoid mixing the two assignment principles. But it is ok to do something like: cnt <= cnt + 1; if (cnt > 100) cnt <= 0; If not, how else should something like this be done? Thanks for your help, I'm finding HDL to be anything but easy. :-)
      - Taniwha 8y ago
        probably not a good idea, because you are sampling cnt after think you have incremented it and you will get the wrong value Essentially what happens is: tmp_cnt1 = cnt + 1; if (cnt > 100) tmp_cnt2 = 0; some time later: cnt = tmp_cnt1; and after that (if cnt >100) cnt = tmp_cnt2; Better to say: if (cnt >= 99) { cnt <= 0; } else { cnt <= cnt+1; }
        hrvach 8y ago
        Thanks for pointing it out, I'll try to stick to a simple if-else construct in the future!
        FullyFunctional 8y ago
        There is certainly no consensus that it's the preferred style. Some designers prefer this explicit (and more verbose) style but others prefer the less verbose one (with default first, business logic next, and reset values at the end).
tasty_freeze 8y ago
Here is another suggestion that is part stylistic, but may save you debugging effort. When performing boolean condition testing, eg "if (~a & b)", always use "!" for negation instead of "~", "&&" for logical and, and "||" for logical or for exactly the same reasons you should do it for C code. If the operands are all 1 bit, either option works fine. But if you ever accidentally use a vector where you intended a bit, one generates a warning and the other doesn't.

jhallenworld 8y ago

I made a beginners guide to Verilog always blocks and assignment statements that talks about this very thing: https://www.youtube.com/watch?v=0pyQ0ER9Rqc https://www.youtube.com/watch?v=0pyQ0ER9Rqc