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

SEQNG-123: Base implementation of FITS keywords. #161

Merged
merged 1 commit into from
Feb 3, 2017

Conversation

jluhrs
Copy link
Contributor

@jluhrs jluhrs commented Feb 2, 2017

I had to scale down the scope of the task. Now it only include the infrastructure that will allow me to add the keywords.
I separated the writing of the keywords to the DHS from the reading of the values. That way I can test without accessing the EPICS systems.

@@ -0,0 +1,1046 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the import of the Channel Access definition file used for the DHS keywords in the old Seqexec. Not yet used, but still included for future work.

def buildBoolean(get: SeqAction[Boolean], name: String ): KeywordBag => SeqAction[KeywordBag] = buildKeyword(get, name, BooleanKeyword)
def buildString(get: SeqAction[String], name: String ): KeywordBag => SeqAction[KeywordBag] = buildKeyword(get, name, StringKeyword)

private def bundleKeywords(inst: String, ks: Seq[KeywordBag => SeqAction[KeywordBag]]): SeqAction[KeywordBag] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure that in a month, I will not understand this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may not be pretty, but it can be guessed what it does. Why the the get parameter and the result of build... are wrapped in a SeqAction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because most of the time the action of retrieving a value involves reading from an EPICS channel, and it can fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may need some shapeless there 😄

* Created by jluhrs on 1/31/17.
*/

trait TcsKeywordsReader {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More values to come. It is not yet used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider to have a single get[A](name: String):A type of method instead?

Copy link
Contributor Author

@jluhrs jluhrs Feb 3, 2017

Choose a reason for hiding this comment

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

Yes, but decided against it. The implementation of get[A](name: String):A would still need to switch between all the possible values of name, so it is not less code. And misspelling a name would be a run time error instead of a compile time error.

Copy link
Contributor

@jdnavarro jdnavarro left a comment

Choose a reason for hiding this comment

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

What are keywords for? I guess they can only be obtained from the underlying EPICS channels, right?

def buildBoolean(get: SeqAction[Boolean], name: String ): KeywordBag => SeqAction[KeywordBag] = buildKeyword(get, name, BooleanKeyword)
def buildString(get: SeqAction[String], name: String ): KeywordBag => SeqAction[KeywordBag] = buildKeyword(get, name, StringKeyword)

private def bundleKeywords(inst: String, ks: Seq[KeywordBag => SeqAction[KeywordBag]]): SeqAction[KeywordBag] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may not be pretty, but it can be guessed what it does. Why the the get parameter and the result of build... are wrapped in a SeqAction?

Copy link
Contributor

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

Added some random comments

@@ -0,0 +1,1046 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this XML machine generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was created running a program over the file that defined the channels in the old Seqexec. Then I had to change the types by hand, because the old format doesn't have type information. So yes, the file is machine generated, but not in the sense that it can be generated at build time, if that is what you were thinking.

for {
id <- client.createImage(DhsClient.ImageParameters(DhsClient.Permanent, List("flamingos2", "dhs-http")))
id <- dhs.createImage(DhsClient.ImageParameters(DhsClient.Permanent, List(instContributorName, "dhs-http")))
_ <- headers.map(_.sendBefore(id, dhsInstrumentName)).sequenceU
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a call to the dhs for each header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

_ <- EitherT(Task {
Log.log(Level.INFO, name + ": starting observation " + id)
Thread.sleep(5000)
Log.log(Level.INFO, name + ": observation completed")
TrySeq(())
})
_ <- client.setKeywords(id, DhsClient.KeywordBag(
_ <- dhs.setKeywords(id, DhsClient.KeywordBag(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these keywords hardcoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. That GMOS code is just a place holder.

def buildBoolean(get: SeqAction[Boolean], name: String ): KeywordBag => SeqAction[KeywordBag] = buildKeyword(get, name, BooleanKeyword)
def buildString(get: SeqAction[String], name: String ): KeywordBag => SeqAction[KeywordBag] = buildKeyword(get, name, StringKeyword)

private def bundleKeywords(inst: String, ks: Seq[KeywordBag => SeqAction[KeywordBag]]): SeqAction[KeywordBag] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

You may need some shapeless there 😄

@@ -9,7 +9,7 @@ import scalaz.concurrent.Task
trait Instrument extends System {
// The name used for this instrument in the science fold configuration
val sfName: String
def observe(config: Config): SeqObserve[DhsClient, ObserveResult]
def observe(config: Config): SeqObserve[(DhsClient, List[Header]), ObserveResult]
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we need to link this to the client level request to, to send client headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not here. There is an ObsKeywordsReader trait used to get those values. The concrete must get them from the client.

* Created by jluhrs on 1/31/17.
*/

trait TcsKeywordsReader {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider to have a single get[A](name: String):A type of method instead?

Copy link
Contributor

@cquiroz cquiroz 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 to me 👍

@jluhrs jluhrs merged commit 17ea2fb into gemini-hlsw:develop Feb 3, 2017
@jluhrs jluhrs deleted the feature/SEQNG-123 branch February 3, 2018 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants