accellera-official / cci

SystemC Configuration, Control and Inspection (CCI)
https://systemc.org/overview/systemc-cci/
Apache License 2.0
10 stars 11 forks source link

Improve definition (and doc) of parameter value states. #26

Closed twieman closed 6 years ago

twieman commented 9 years ago

The names "default" and "initial" are not intuitive, need to be revisited and fully documented (there are many documentation gaps but these notions are very central). Invalid values should be addressed too - and inclusion of an explicit invalid value for each type reconsidered. See Tom's feedback document for suggestions. Ballpark effort estimate: 1 week.

twieman commented 6 years ago

During our 8/10/2017 WG meeting we agreed that "constructor value" and "broker value" seem better terminology.

markfoodyburton commented 6 years ago

This is a huge, scary search and replace. It'll touch about every file in the repo. I'd suggest we need to co-ordinate this...

pah commented 6 years ago

This is a huge, scary search and replace. It'll touch about every file in the repo. I'd suggest we need to co-ordinate this...

Agreed. But it might not even be a simple search and replace, as we'd end up with a function like:

  cci_broker_handle broker;
  broker.set_broker_cci_value( "foo.param", cci_value(42) );

Setting a "broker value" via a "broker handle" sounds very confusing to me.

On a parameter, queries work fine this way:

  cci_param_untyped_handle param /* = ... */;
  if (param.has_broker_value())
    /*...*/ param.get_broker_value();

Maybe we can find a different name for the broker-side calls?

markfoodyburton commented 6 years ago

I agree - I'm sure some s/r will help, but this is a more 'nuanced' change, it will have to be highly manual. Equally - I think your point is a good one:

"Broker set initial cci value" is clear (ish) "Broker set broker cci value" is not.... it seems to be (at best) "Broker set cci value", which implies your setting the value "now" not the "initial" value.....

I think we all agree the names are 'wrong' and 'constructor' and 'broker' would be good, but "broker set broker value" is really bad..... needs more discussion :-(

markfoodyburton commented 6 years ago

lest we forget - I think @pah suggested "set_prevalue" for the broker (instead of set_broker_value)...

twieman commented 6 years ago

More specifically, I understood Philipp's proposal was to change set_initial_cci_value() to preset_initial_cci_value(). Further, we would adopt the terminology of referring to this as the parameter's "broker" value vs. the "constructor" value supplied at object creation.

markfoodyburton commented 6 years ago

I'd like 'initial' dropped from that : my_broker->preset_cci_value("foo.baa", cci_value(42)) I think "preset" covers the meaning added by "initial"....

ccigit commented 6 years ago

Have we considered the term “configure”? And why say “cci”? Clearly, we are very much in CCI land when making this call.

g_broker->configure_value(“foo.baa”, cci_value(24));

From: cciwg@lists.accellera.org [mailto:cciwg@lists.accellera.org] On Behalf Of markfoodyburton (via GitHub) Sent: Friday, September 01, 2017 11:10 AM To: Accellera SystemC CCIWG Subject: [cciwg] Re: [OSCI-WG/cci] Improve definition (and doc) of parameter value states. (#26)

EXTERNAL MAIL

I'd like 'initial' dropped from that : my_broker->preset_cci_value("foo.baa", cci_value(42)) I think "preset" covers the meaning added by "initial"....

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_OSCI-2DWG_cci_issues_26-23issuecomment-2D326493589&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=wGlr_RPcXH-BxuLIlw4f0-S9bUepkw-EZNSzvYU6eMo&m=1cfHT3rNi2iL1l8GBfQp4noNr0bjzoHpvkdYm3fXbJE&s=q0y0Q8L-ZLL9_qAM3Bwpk3q1oVLneCXjg40J4TqJo2w&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ADSs-2DDDkiXE6cgpLdkWMBtPNW-5FSyf1shks5sd5ibgaJpZM4C-5FTht&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=wGlr_RPcXH-BxuLIlw4f0-S9bUepkw-EZNSzvYU6eMo&m=1cfHT3rNi2iL1l8GBfQp4noNr0bjzoHpvkdYm3fXbJE&s=QY93_UXUK2ibWdMHtbVAVqw9JFMcn7oWsxauqnoI69A&e=.

pah commented 6 years ago

Thanks for the suggestion, Bishnupriya!

I also thought about configure_ before and could live with that as well. My only concern about

  broker_handle.configure_value("foo.bar", cci_value(42) );

is that it conveys the "initialization-only semantics" less clearly. Why shouldn't you interpret a runtime update of a parameter as a (re)configuration as well? That's how I ended up with preset_value(...) instead.

That said, I'm now uncertain that a "constructor value" is actually more clear than a "default value", because the inital/preset/configured/broker value is also applied during construction (if present).

markfoodyburton commented 6 years ago

BTW, I thing that the reason we have the 'cci_value' in there is because you have to provide a 'cci_value' type, rather than it being part of the cci library.

For the names - I despair, nothing fits quite right....

twieman commented 6 years ago

@markfoodyburton is right I intended to write preset_cci_value().

@pah summarizes our prior exploration of configure_ very well. Once we add the ability to call broker_handle.set_cci_value("foo.bar", cci_value(42)); (see #174) the need to clearly distinguish initialization-only becomes bigger.

We've attempted to consistency use *_cci_value when the argument type is cci_value, *_value when the argument type is <T>, and *_raw_value when the argument type is void *.

twieman commented 6 years ago

Is it too much of a mouthful to use the terms "broker preset value" and "constructor argument value"?

twieman commented 6 years ago

Yes that's too much of a mouthful. After giving it some more thought I really think good ole "default value" works fine. At parameter construction we provide a default value that's used when there's no preset value available. I'm even starting to like the simplicity of a "preset value" that comes from the broker vs. referring to it as the "broker value".

twieman commented 6 years ago

This was inadvertently closed (by PR #190) and remains an open issue.