ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

Impure function called from pure function MemoryBase.getMemory

Open MarkusOlssonModelon opened this issue 4 years ago • 1 comments

I have found an issue in Modelica.Electrical.Digital.Interfaces.MemoryBase.getMemory. It contains a function call to Modelica.Utilities.Streams.readLine which is an impure function, but getMemory is not declared as impure.

I suggest adding the impure prefix to getMemory. This will also require changes in Modelica.Electrical.Digital.Memories.DLATRAM and Modelica.Electrical.Digital.Memories.DLATROM since they contain calls to getMemory. For that I think the simplest solution is to change the if-statements surrounding the calls to when-statements.

MarkusOlssonModelon avatar Aug 09 '21 09:08 MarkusOlssonModelon

The functiongetMemory should not be declared as impure, but it should be handled in another way. Basically the function getMemory is intended to be behave in a pure way; and it is primarily used with tables given in the library.

HansOlsson avatar Aug 09 '21 09:08 HansOlsson

Looking at it once more I think that purity is the least of our concerns.

Superficially it seems to do the following:

Fill a logical table with binary values, 0, 1, 2, ... 2^(n_addr)-1, with n_data bits from left-to-right, stored as L.'0', L.'1' (and L.'X' for the left-over bits).

For some reason that is done using an external table which makes it impure, and fails to handle n_addr>4 and if n_data>4 the rest of the bits are left-over bits.

So, if we don't change the external table, I would have the following questions:

  • Is the intent to handle n_data<n_addr at all? (Otherwise an assert would make sense.)
  • Is the intent that n_data>n_addr should generate left-over bits, or if there's something special for n_data=4?
  • Is the intent only handle n_addr<=4, or if it should be rewritten to handle any value for n_addr?

If the intent is to provide a different external table the obvious improvement would be to have a table in Modelica instead, so that you don't have to set n_addr, n_data, and external table separately.

And note that this is just how the memory is initialized (the RAM-model allows writing as well). Assuming that the models are useful I would believe it would make more sense to have a selectable memory-initialization-function; like generalization of current one, fill with some value (it seems the original model just filled with 'U' - as the start-value), or user-selectable.

HansOlsson avatar Sep 05 '23 14:09 HansOlsson