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

Device code incompatibilty issue #251

Closed
PeterCoghlan opened this issue Sep 19, 2019 · 9 comments
Closed

Device code incompatibilty issue #251

PeterCoghlan opened this issue Sep 19, 2019 · 9 comments
Assignees
Labels
Discussion Developers are invited to discuss a design change or solution to a coding problem. Enhancement This issue does not describe a problem but rather describes a suggested change or improvement.

Comments

@PeterCoghlan
Copy link

PeterCoghlan commented Sep 19, 2019

The type of the tenth element of the DEVHND structure (the address of the device halt function = typedef BYTE DEVHF (DEVBLK *dev)) is different to the type used in the non-SDL Hyperion variant (typedef void DEVSF (DEVBLK *dev)). This is declared near the top of devtype.h or in htypes.h depending on point of view.

If the type was changed back from BYTE to void, it would restore a certain amount of compatibility of device code between the two Hyperion variants.

As far as I can see, the device halt function is only called from channel.c and the value returned by the device halt function is not tested when it is called. Therefore, unless I missed something, I think that changing it's type to void should not cause any change in behaviour.

Can this be done?

(If it is possible to change the type back, the devices whose halt functions return values would also need to be tweaked to not return values.)

@Fish-Git
Copy link
Member

Fish-Git commented Sep 20, 2019

If the type was changed back from BYTE to void, it would restore a certain amount of compatibility of device code between the two Hyperion variants.

And this is important because . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . Why?

As far as I can see, the device halt function is only called from channel.c and the value returned by the device halt function is not tested when it is called. Therefore, unless I missed something, I think that changing it's type to void should not cause any change in behaviour.

That is true . . . . today. But technically the channel.c code that calls that function should be checking the return code. It's not today, but technically it should be. According to Principles of Operation, the behavior of Clear- and Halt-Function Signaling and Completion is different depending on whether the attempt to issue the Clear/Halt Signal is successful or not, and you can't tell whether the attempt was successful or not without the function returning a return code. That is to say, a void function doesn't tell you whether the function successfully did what you wanted it to do or not. Hence the reason behind my changing its type to BYTE. (It could probably be changed to bool instead of BYTE, but either way the caller is able to learn whether the request was successful or not, which, as I said, you can't do with a void type.)

To answer your question directly, sure, it can be easily changed back. But why should it be? Your apparent goal of wishing to remain compatible with an old unmaintained fork of Hercules is misplaced IMO, and only serves to limit the freedom the only current/actively maintained version of Hercules currently enjoys, and I personally don't like having existing freedoms taken from me.

Others may of course feel differently and to that end I heartily encourage my fellow team members to please offer their own thoughts regarding this issue.

@Fish-Git Fish-Git added the Discussion Developers are invited to discuss a design change or solution to a coding problem. label Sep 20, 2019
@wably
Copy link
Member

wably commented Sep 20, 2019

Hi Fish,

You make an excellent case as to why the return code should be made available for checking. Here is my two cents, since you asked.

The incompatibility issue becomes a problem for those that are writing device code, if they wish to have their code compile on different versions of the emulator. It makes it difficult to have device code compile on Hercules 3.13 or regular Hyperion 4.0 for example, and still compile on SDL Hyperion. Peter likely asked for this to be resolved for his device handler. Peter Jansen may also face the same issue for his new and updated CTCE code if he chooses to make it available in the older versions as well.

You may wonder why anyone would want do run the older versions but the fact is that many still do. So it would be nice if developers could be able to have less code and syntactical incompatibilities where possible. I realize it would not be possible in every case. But maybe for this one there are some other options. Maybe a #ifdef or something to indicate the difference so one could compile with a single version of their source code on either Hercules/Hyperion version. I don't want to speak for either Peter as to what solution would be acceptable to them, but that is one approach I think.

Again, just my two cents.

Regards,
Bob

@Fish-Git Fish-Git added the Enhancement This issue does not describe a problem but rather describes a suggested change or improvement. label Sep 20, 2019
@PeterCoghlan
Copy link
Author

Compatibility is very important to me.

I think that what has enabled mainframe technology to endure for so long is the emphasis that has been placed on maintaining compatibilty with what has gone before when advances are made in hardware or software. VM/370 and MVS 3.8j are much more old and unmaintained than any version of Hercules that exists now, yet we are still able to make considerable use of them today, mainly because everything that was developed after these operating systems remains compatible with what came before.

While we all have freedoms to make changes to old interfaces to add new features or enhance their function in whatever way we see fit, I think life becomes so much more tolerable if we take great care to maintain as much compatibility as possible with the way things were done previously when we make these changes. For example, instead of modifying an element in the middle of an array which forms part of an important interface within Hercules which has established use, a new, enhanced element which behaves slightly differently to the element which is found wanting can be added at the end of the array. The old element can continue to support code that it previously supported and the new element can provide the required enhanced support to new code which has been newly designed to take advantage of it.

(Ideally, the interface between Hercules and Hercules devices would use something more flexible than a simple array of functions, something containing version information, something that can easily be expanded when required, however, we are where we are with this and I believe best results will be obtained by sticking as closely as possible to what exists already.)

@Fish-Git
Copy link
Member

Fish-Git commented Sep 21, 2019

For example, instead of modifying an element in the middle of an array which forms part of an important interface within Hercules which has established use, a new, enhanced element which behaves slightly differently to the element which is found wanting can be added at the end of the array. The old element can continue to support code that it previously supported and the new element can provide the required enhanced support to new code which has been newly designed to take advantage of it.

Point taken.

At the time I made the change however, I wasn't concerned with being compatible with non-SDL versions of Hyperion.

Further, I'm still somewhat confused as to what the big deal is. How difficult is it to typedef the DEVHF function type differently depending on which Hyperion your code is being compiled for (and to return a return code for one but not for the other). That sounds rather trivial to do IMO. So as I said, I still don't understand what the big deal is. I still don't understand why you're pushing so hard for this change.

I can easily change the halt vector back to DEVSF type and add the new DEVHF type to the end of the array as you suggested (and that is ultimately what I will very likely end up doing only because I don't wish to alienate an important long term and valued contributor to Hercules), but as I said I still don't understand WHY it's so damn important that I do so!

I just don't get it. I understand the argument, I just don't understand why you're making it.

@Fish-Git
Copy link
Member

Here is my two cents, since you asked.

It makes it difficult to have device code compile on Hercules 3.13 or regular Hyperion 4.0 for example, and still compile on SDL Hyperion.

Oh come on! How difficult is it to add a #ifdef to typedef the DEVHF function type differently depending on which Hyperion your code is being compiled for? (And to either return a return code value or nothing at all in your device halt function?) I mean, I would have to seriously question the coding skills of any programmer that considered doing that to be "difficult"! :)

@Fish-Git
Copy link
Member

Fish-Git commented Sep 21, 2019

(Ideally, the interface between Hercules and Hercules devices would use something more flexible than a simple array of functions, something containing version information, something that can easily be expanded when required . . .

I absolutely agree. I knew it was a mistake when I first saw it. Jan certainly didn't give it much thought when he coded (designed) it, that's for sure. (I'm presuming it was Jan.) If it were me, I would have designed something closer to the ioctl function.

@Fish-Git Fish-Git self-assigned this Sep 22, 2019
@Fish-Git Fish-Git added the IN PROGRESS... I'm working on it! (Or someone else is!) label Sep 22, 2019
@Fish-Git
Copy link
Member

@PeterCoghlan
@wably

Peter,

MEA CULPA!

It appears the entire reason for my having originally changed the type of the DEVHF halt function from void to BYTE was flawed!

I was incorrectly interpreting what the Principles of Operation was actually saying on the subject!

 
SA22-7832-11:

page 15-16:

Clear-Function Signaling and Completion
The Attempt to Issue the Clear Signal Is Not Successful:

When the channel subsystem ATTEMPTS to issue the clear signal to the device, the ATTEMPT may not be successful ...

If ... the channel subsystem either determines that the ATTEMPT to issue the clear signal was not successful or cannot determine whether the ATTEMPT was successful, the subchannel remains clear pending and is set status pending, and the performance of the clear function is complete.

 
page 15-18:

Halt-Function Signaling and Completion
The Attempt to Issue the Halt Signal Is Not Successful:

When the channel subsystem ATTEMPTS to issue the halt signal to the device, the ATTEMPT may not be successful ...

If ... the channel subsystem either determines that the ATTEMPT to issue the halt signal was not successful or cannot determine whether the ATTEMPT was successful, then the subchannel remains halt pending and is set status pending, and the halt function is complete.

 
The key takeaway is the verbage used: "...when the ATTEMPT to issue the clear/halt SIGNAL is not successful...".

In other words, if its attempt to REQUEST that the device be halted or cleared fails, THEN blah, blah, blah.

But as long as it was able to successfully REQUEST (ask) the device to perform the halt or clear, then its ATTEMPT (to signal the device) was successful!

Whether the device was able to actually successfully perform the halt or clear is immaterial! The only thing that matters is whether or not the channel's ATTEMPT to ask the device to halt or clear was successful or not.

And in the case of Hercules, as long as the halt function exists in the array (and can thus be called), the channel's attempt (to ask the device to perform its halt/clear function) is guaranteed to always be successful!

When I originally read the Principles of Operation I was thinking about what would happen if the device was unable to halt or clear itself, but that doesn't really matter. The DEVICE'S attempt to halt/clear itself can fail or succeed, but the channel doesn't care about that. The only thing the channel cares about is whether its REQUEST (signal) made it to the device or not (i.e. whether or not the device was ASKED to perform the halt/clear). Whether the actual device halt/clear was successful or not is largely immaterial.

Sooo.... what this means to you is, I owe you an apology. You were right and I was wrong. I made a change that turns out to have been completely unnecessary! The DEVHF halt/clear function typedef can (should!) remain void just like it originally was.

I will be committing the fix for you shortly.

@PeterCoghlan
Copy link
Author

Many thanks Fish.

The reason I was so keen to have this issue resolved is:

I have written some device code for Hercules. A few years ago, my code could cope with operating in Hercules 3.x environment or a Hyperion environment. At that time, this was the entire Hercules universe but this is no longer the case.

I find it reasonably possible to cope with two differing platforms with common code but trying to cope with a third one as well is stretching my juggling abilities.

@Fish-Git
Copy link
Member

Fixed by commit bc97ba8.

Closing.

@Fish-Git Fish-Git removed the IN PROGRESS... I'm working on it! (Or someone else is!) label Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Developers are invited to discuss a design change or solution to a coding problem. Enhancement This issue does not describe a problem but rather describes a suggested change or improvement.
Projects
None yet
Development

No branches or pull requests

3 participants