From 0477c7e042aee72fbd2afb722e68f6dee61c102c Mon Sep 17 00:00:00 2001 From: tobygomersall Date: Mon, 4 Jul 2022 18:10:21 +0100 Subject: [PATCH] =?UTF-8?q?[ENH]=20Updated=20the=20ConcatSignal=20conversi?= =?UTF-8?q?on=20code=20to=20check=20for=20undriven=20=E2=80=A6=20(#371)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [ENH] Updated the ConcatSignal conversion code to check for undriven signals. * [ENH] Added a test to check the undriven ConcatSignal converts sensibly. --- myhdl/_ShadowSignal.py | 74 ++++++++++++++++++- .../conversion/general/test_ShadowSignal.py | 18 +++++ 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/myhdl/_ShadowSignal.py b/myhdl/_ShadowSignal.py index 430721cb..40d49aad 100644 --- a/myhdl/_ShadowSignal.py +++ b/myhdl/_ShadowSignal.py @@ -196,8 +196,39 @@ class ConcatSignal(_ShadowSignal): else: w = len(a) lo = hi - w + + if isinstance(a, _Signal) and a._name is None: + # We have seen a bug when a concat signal is created in the + # following way: + # + # sig_list = [Signal(False) for n in range(32)] + # concat_sig = ConcatSignal(*reversed(sig_list)) + # + # It seems that the _name attribute on the signals in sig_list + # is only updated if an assignment is made to them. Otherwise + # _name is left as None. We need to check for None and raise + # a warning. + from myhdl.conversion._misc import _error + from myhdl import ToVHDLWarning + + if w == 1: + warnings.warn( + "%s: %s[%s]" % (_error.UndrivenSignal, self._name, lo), + category=ToVHDLWarning) + else: + warnings.warn( + "%s: %s[%s:%s]" % (_error.UndrivenSignal, self._name, hi, lo), + category=ToVHDLWarning) + if w == 1: - if isinstance(a, _Signal): + if isinstance(a, _Signal) and a._name is not None: + # Check that a._name is not None as None should not be + # written into the converted code. If it is None then we + # assume no assignment has been made to the signal + # (otherwise the _name attribute would have been updated + # by the _analyzeSigs function). In this situation the + # signal should hold its init value (as handled in the + # else branch). if a._type == bool: # isinstance(a._type , bool): <- doesn't work lines.append("%s(%s) <= %s;" % (self._name, lo, a._name)) else: @@ -205,7 +236,9 @@ class ConcatSignal(_ShadowSignal): else: lines.append("%s(%s) <= '%s';" % (self._name, lo, bin(ini[lo]))) else: - if isinstance(a, _Signal): + if isinstance(a, _Signal) and a._name is not None: + # Check that a._name is not None as None should not be + # written into the converted code lines.append("%s(%s-1 downto %s) <= %s;" % (self._name, hi, lo, a._name)) else: lines.append('%s(%s-1 downto %s) <= "%s";' % @@ -223,8 +256,39 @@ class ConcatSignal(_ShadowSignal): else: w = len(a) lo = hi - w + + if isinstance(a, _Signal) and a._name is None: + # We have seen a bug when a concat signal is created in the + # following way: + # + # sig_list = [Signal(False) for n in range(32)] + # concat_sig = ConcatSignal(*reversed(sig_list)) + # + # It seems that the _name attribute on the signals in sig_list + # is only updated if an assignment is made to them. Otherwise + # _name is left as None. We need to check for None and raise + # a warning. + from myhdl.conversion._misc import _error + from myhdl import ToVerilogWarning + + if w == 1: + warnings.warn( + "%s: %s[%s]" % (_error.UndrivenSignal, self._name, lo), + category=ToVerilogWarning) + else: + warnings.warn( + "%s: %s[%s:%s]" % (_error.UndrivenSignal, self._name, hi, lo), + category=ToVerilogWarning) + if w == 1: - if isinstance(a, _Signal): + if isinstance(a, _Signal) and a._name is not None: + # Check that a._name is not None as None should not be + # written into the converted code. If it is None then we + # assume no assignment has been made to the signal + # (otherwise the _name attribute would have been updated + # by the _analyzeSigs function). In this situation the + # signal should hold its init value (as handled in the + # else branch). if a._type == bool: lines.append("assign %s[%s] = %s;" % (self._name, lo, a._name)) else: @@ -232,7 +296,9 @@ class ConcatSignal(_ShadowSignal): else: lines.append("assign %s[%s] = 'b%s;" % (self._name, lo, bin(ini[lo]))) else: - if isinstance(a, _Signal): + if isinstance(a, _Signal) and a._name is not None: + # Check that a._name is not None as None should not be + # written into the converted code lines.append("assign %s[%s-1:%s] = %s;" % (self._name, hi, lo, a._name)) else: lines.append("assign %s[%s-1:%s] = 'b%s;" % diff --git a/myhdl/test/conversion/general/test_ShadowSignal.py b/myhdl/test/conversion/general/test_ShadowSignal.py index 9808c6d9..ff6baae8 100644 --- a/myhdl/test/conversion/general/test_ShadowSignal.py +++ b/myhdl/test/conversion/general/test_ShadowSignal.py @@ -103,6 +103,24 @@ def bench_ConcatSignalWithConsts(): def test_ConcatSignalWithConsts(): assert conversion.verify(bench_ConcatSignalWithConsts()) == 0 +@block +def bench_ConcatSignalUndriven(): + + n_sigs = 32 + + sig_list = [Signal(False) for n in range(n_sigs)] + concat_sig = ConcatSignal(*reversed(sig_list)) + + @instance + def check(): + + yield delay(10) + print(concat_sig) + + return check + +def test_ConcatSignalUndriven(): + assert conversion.verify(bench_ConcatSignalUndriven()) == 0 @block def bench_TristateSignal():