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

Annotate and BiSignal don't work together #2472

Closed
lonetech opened this issue May 21, 2023 · 8 comments · Fixed by #2475
Closed

Annotate and BiSignal don't work together #2472

lonetech opened this issue May 21, 2023 · 8 comments · Fixed by #2475
Labels

Comments

@lonetech
Copy link

When using Annotate to attach attributes to a BiSignalIn, it gets incorrectly declared as input. Example (comment out the Annotate lines to see the correct inout declaration):

module BiSigBug where

import Clash.Prelude
import Clash.Annotations.SynthesisAttributes

-- BiSignalIn must be present at the top level.
-- Putting it inside a multifield record caused the assign to drive an internal wire. 
type FtdiIO dom = "FTDI_D" ::: BiSignalIn 'Floating dom 8
                  `Annotate` 'StringAttr "LOC" "P1, P2, P4, N5, P5, M6, N6, M7"
                  `Annotate` 'StringAttr "IOSTANDARD" "LVTTL"
                  -- FIXME: Annotations break BiSignal in Clash 1.6.4!

topEntity
  :: Clock System -- -> Reset System -> Enable System
  -> FtdiIO System -> BiSignalOut 'Floating System 8
topEntity clk io
  = withClockResetEnable clk rst en $ writeToBiSignal io (pure out)
  where
    out = Nothing :: Maybe (BitVector 8)
    rst = unsafeToReset $ pure False -- resetGen
    en = enableGen
@christiaanb
Copy link
Member

Thanks for the report!

Should be fixed by: #2475

@christiaanb
Copy link
Member

With the above fix I get the following VHDL:

-- Automatically generated VHDL-93
library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;
use IEEE.MATH_REAL.ALL;
use std.textio.all;
use work.all;
use work.BiSigBug_topEntity_types.all;

entity topEntity is
  port(-- clock
       clk : in BiSigBug_topEntity_types.clk_System;
       io  : inout std_logic_vector);

  attribute LOC : string;
  attribute LOC of io : signal is "P1, P2, P4, N5, P5, M6, N6, M7";

  attribute IOSTANDARD : string;
  attribute IOSTANDARD of io : signal is "LVTTL";
end;

architecture structural of topEntity is


begin
  -- writeToBiSignal# begin
  io <= (std_logic_vector'("--------")) when false else (8-1 downto 0 => 'Z');
  -- writeToBiSignal# end


end;

@leonschoorl
Copy link
Member

       io  : inout std_logic_vector);

Aren't we missing the size of the std_logic_vector there?

christiaanb added a commit that referenced this issue May 22, 2023
@christiaanb
Copy link
Member

Good catch, I fixed that as well now.

christiaanb added a commit that referenced this issue May 22, 2023
christiaanb added a commit that referenced this issue May 22, 2023
@lonetech
Copy link
Author

Code changes look good to me, and I've test run as well.

I'm probably doing something else wrong as my self-built (git clone+stack build+stack run clash) version usually crashes with a memory error, but that seems unrelated to this change.

With this fixed I'll be able to build Xilinx projects without separate constraint files (messing with a miniSpartan6+ right now, got it talking to the USB port and LEDs).

@DigitalBrains1
Copy link
Member

usually crashes with a memory error

You're probably experiencing GHC bug #19421. Try passing -with-rtsopts=-xm20000000 when compiling an affected binary (we do this for CI in #2444 on affected tests). It's really annoying, but we can't really do anything about it. It might be the RTS option is harmless enough to just always pass it, but that's a bit difficult to gauge based on the available information. The documentation definitely warns not to do it :-).

@lonetech
Copy link
Author

AFAICT -xm uses MAP_FIXED, and can overwrite any maps that happen to be there, however it's likely to be none. And the root bug seems to be in the Linux kernel; MAP_32BIT fails unnecessarily. The workaround should use MAP_FIXED_NOREPLACE for collision detection. I'm going to try a different kernel release. The other workaround with -fPIC -fexternal-dynamic-refs should be reliable, but requires recompiling everything.

@christiaanb
Copy link
Member

We've released v1.8.0, which includes a fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants