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

Merge HeatingNMOS/HeatingPMOS with NMOS/PMOS #3167

Merged
merged 3 commits into from
Oct 28, 2019

Conversation

beutlich
Copy link
Member

See #2899.

@beutlich beutlich added L: Electrical.Analog Issue addresses Modelica.Electrical.Analog task General work that is not related to a bug or feature labels Oct 23, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone Oct 23, 2019
@beutlich beutlich self-assigned this Oct 23, 2019
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is a change hidden here:

  • The old ?MOS had useHeatPort=false;
  • The new ?MOS has useHeatPort=true
  • The old Heating?MOS had useHeatPort=true;
    As I understand that means that old models that used a ?MOS without changes will now have a dangling heatPort - which will imply a singularity (since the temperature isn't given).

I believe that the right approach is to have useHeatPort=false for the new ?MOS, which implies that the example model will still need useHeatPort=true.

There are other alternative, but I believe they have more complicated conversions.

@beutlich
Copy link
Member Author

I guess, this also was overlooked in #3010 then. Easiest would be to set useHeatPort=useTemperatureDependency in the new models, right?

@HansOlsson
Copy link
Contributor

I guess, this also was overlooked in #3010 then. Easiest would be to set useHeatPort=useTemperatureDependency in the new models, right?

That is one possibility, I tried to list all of them below:

  • Don't set useHeatPort, let the user set both. Seems non-intuitive.
  • Don't set useHeatPort, let the user set both; but make useTemperatureDependency conditionally enabled on useHeatPort. However, that means we cannot use a static temperature dependency.
  • Add: useHeatPort=useTemperatureDependency (still allows all variant, but convenient default)
  • Add: useTemperatureDependency=useHeatPort (still allows all variant, but convenient default)

To me the heat-port seems like the primary parameter, and thus the last option seems preferable; but I'm not an electrical engineer so I don't know which parameter is most natural.

@beutlich
Copy link
Member Author

beutlich commented Oct 25, 2019

If we have useTemperatureDependency=useHeatPort and have useHeatPort=true (as new default) we do not get the old Diode/PMOS/NMOS behavior when using that new model (of v4.0.0).
But having useHeatPort=useTemperatureDependency and useTemperatureDependency=false (as default) and setting useTemperatureDependency=true when converting HeatingDiode etc. to Diode, we consider all use-cases correctly.

@HansOlsson
Copy link
Contributor

If we have useTemperatureDependency=useHeatPort and have useHeatPort=true (as new default) we do not get the old Didoe/PMOS/NMOS behavior when using that new model (of v4.0.0).
But having useHeatPort=useTemperatureDependency and useTemperatureDependency=false (as default) and setting useTemperatureDependency=true when converting HeatingDiode etc. to Diode, we consider all use-case correctly.

Yes, I had just realized that as well.

It's just that to me it seems natural to enable both temperature dependency and heat-port (if you want either of them), and thus that should be natural in the user-interface; i.e. the natural user experience is to set useTemperatureDependency. I don't know how we can do that in a simple way.

@beutlich
Copy link
Member Author

beutlich commented Oct 25, 2019

Parameters useTemperatureDependency and useHeatPort are independent, actually. useTemperatureDependency=false was introduced as new parameter for v4.0.0. Having useHeatPort=useTemperatureDependency enables a clean conversion. Users still can set useHeatPort to false/true since it is no final modification.

@beutlich
Copy link
Member Author

Is it actually a problem that useHeatPort is shown as check box in the UI?

@beutlich
Copy link
Member Author

There is a small UI issue in Dymola 2020 with depending parameters. This is how the new NMOS properties dialog looks:

grafik

If useTemperatureDependency is set to true, then implicitly useHeatPort is set to true implying that T should be drawn disabled (which is not the case):

grafik

@HansOlsson
Copy link
Contributor

HansOlsson commented Oct 28, 2019

Dymola uses a tri-state checkbox if its neither explicitly true nor false, and in this case the value is useTemperatureDependency (which happens to be false). Using the normal two-state would make it graphically difficult to see if a user changed the value between false and useTemperatureDependency.

Obviously we could use a different marker, but that adds complexity.

However, the point I wanted to make earlier was something else: can we make it easier for users to understand that it would be good to change useTemperatureDependency instead of useHeatPort?

I don't see any amazing solutions, so just having it "earlier" in the dialog (as in this case), and reusing the logic in other models seems like the way forward.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@beutlich
Copy link
Member Author

Dymola uses a tri-state checkbox if its neither explicitly true nor false

Which is a nice idea to avoid confusion.

If useTemperatureDependency is set to true, then implicitly useHeatPort is set to true implying that T should be drawn disabled (which is not the case)

Still, you need to update the UI if the value of the tri-state checkbox changes due to changes of the connected parameter.

@beutlich beutlich merged commit 0226109 into modelica:master Oct 28, 2019
@beutlich beutlich deleted the merge-MOS branch October 28, 2019 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Electrical.Analog Issue addresses Modelica.Electrical.Analog task General work that is not related to a bug or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants