tuura / plato

A DSL for asynchronous circuits specification
Other
12 stars 2 forks source link

Signal types can now be declared. #38

Closed jrbeaumont closed 8 years ago

jrbeaumont commented 8 years ago

This works with #17.

jrbeaumont commented 8 years ago

@snowleopard: I have just noticed an issue. If a signal type is not declared (i.e. it stays as Unused) then it will not be entered as a signal to the remaining .g file. This means the concepts cannot be correctly translated.

In the Hierarchy of SignalType, I suggest we removed Unused, then every signal can be translated, without having to declare a type. The default case being Internal

snowleopard commented 8 years ago

@jrbeaumont What do you think about calling the new concept is interface instead of signalType? I think it's a bit nicer. I don't like signalType, as it is too low level: type parameter a does not necessarily have to stand for signals in the abstract model. We could also rename SignalType to Interface...

jrbeaumont commented 8 years ago

That would make more sense for the abstract. I'll change it.

With regards to my previous comment, what are your thoughts?

snowleopard commented 8 years ago

I have just noticed an issue. If a signal type is not declared (i.e. it stays as Unused) then it will not be entered as a signal to the remaining .g file.

I'm not sure this is an issue. If the user forgot to specify the type of a signal, then I think this is probably an incorrect specification, so we could check for such cases and report an error to the user.

In the Hierarchy of SignalType, I suggest we removed Unused, then every signal can be translated, without having to declare a type. The default case being Internal

I don't think it will work. If the default value is Internal then you can never get to Input by max. The default value should be lower in the hierarchy than Input.

One possible solution is to use Input as the default. It may be worth discussing this with @danilovesky if he thinks there can be a sensible default. I tend to think that we should keep Unused as the default and report an error during the translation procedure if a signal type has been left unspecified.

jrbeaumont commented 8 years ago

Fair enough. At the moment it is the .g parser in workcraft that throws an exception so I will perform a check in the translate tool and add an error for that. If possible, for clarity, I may add a specific error message in Workcraft for this error, but from the Concepts tool, not the parser.

Do not merge this yet, I'll make these minor changes first.

Final question: In the examples for the buck scenarios, I may move the signals declaration to chargeFunc. I worry this may cause issues with its naming, but it would further prove the reusability of concepts, including the declaration of signal types.

snowleopard commented 8 years ago

If possible, for clarity, I may add a specific error message in Workcraft for this error, but from the Concepts tool, not the parser.

@jrbeaumont Yep, I think this would be ideal.

Think about some circuit concept validation function that does roughly something like:

data ValidationResult a = Valid | UnusedSignal [a] | SomethingElseToBeAddedLater

validate :: [a] -> CircuitConcept a -> ValidationResult a
validate = ...

Then you can go ahead with the translation only if you get Valid as the validation result. Otherwise you report the corresponding error.

In the examples for the buck scenarios, I may move the signals declaration to chargeFunc

Yep, I think this makes sense. By the way, you can also call this interface, not signals.

jrbeaumont commented 8 years ago

@snowleopard I will change all this and add in validate. A quick question, with regards to

validate :: [a] -> CircuitConcept a -> ValidationResult a

Would I need the [a]. Could I not simply perform validation on the CircuitConcept?

snowleopard commented 8 years ago

Would I need the [a]. Could I not simply perform validation on the CircuitConcept?

@jrbeaumont I think you need a list of signals, in order to go through them and check that none of them is Unused. At the moment I think there is no way to extract this list from a CircuitConcept. Maybe there should be.

jrbeaumont commented 8 years ago

Yes I realised while implementing validate.

It works correctly, returning an error stating that one or more signals hasn't had its type declared. Still working on how to say which signal(s).

Maybe there should be a way to get a list of signals directly from a CircuitConcept , but I fear we might need to add signals to the concept type specifically.

snowleopard commented 8 years ago

Still working on how to say which signal(s).

What do you mean? I thought validate will look something like:

validate :: [a] -> CircuitConcept a -> ValidationResult a
validate ss c = case filter ((==Unused) . interface c) ss of
    []     -> Valid
    unused -> UnusedSignal unused

So all you need to do is print the list returned by validate in UnusedSignal.

jrbeaumont commented 8 years ago

That's about what I've done but it's not enjoying the attempt at printing it. I'll try again in the morning and paste an error.

jrbeaumont commented 8 years ago

@snowleopard signals with type not declared will now be output as part of the error. I will add a helpful error message to Workcraft

snowleopard commented 8 years ago

@jrbeaumont I've added a couple of new comments. Please have a look and I think we'll be ready to merge this.

jrbeaumont commented 8 years ago

@snowleopard These changes have been made. Let me know if you've noticed anything else. I'm a bit stuck with initial states. Are you free for 10 minutes this afternoon?

snowleopard commented 8 years ago

@jrbeaumont Thanks! Merged now.