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

Proper implementation of RecoveryAccessor read operations #138

Closed
4 tasks
killenb opened this issue Apr 6, 2020 · 2 comments
Closed
4 tasks

Proper implementation of RecoveryAccessor read operations #138

killenb opened this issue Apr 6, 2020 · 2 comments

Comments

@killenb
Copy link
Member

killenb commented Apr 6, 2020

Child of #123

I had to split it into two phases. Phase 1 is needed for #130, Phase 2 depends on #130 and #131

Note: Phase 1 has already been implemented!

Description of phase 2:
The implementation of #130 adapted to the changes in DeviceAccess (exceptions are not thrown in the transfers but in the post actions), but it does not fully implement the new specification yet. There are three missing features:

  • The first read() or readAsync() on a bad device goes through with old data and DataValidity::invalid. Only the second read()/readAsync() blocks
  • readNonBlocking() /readLatest() never block
  • the initial value is never transported

In addition there is the requirement that no new transfers must be started when the device is recovering, which means the non-blocking reads, which obviously cannot call DeviceModule::waitForRecovery() because it blocks, must not simply try to perform the transfer and let it fail.

Implementation

If nothing else is mentioned it is phase 1 and stays in phase 2.

  • Create a protected variable transferAllowed
  • doPreRead()
    • Don't call wait for recovery here
    • Phase 1 only Set transferAllowed to true if the application is in LifeCycleState::run. Otherwise set it to false.
    • Phase 2 Call DeviceModule::startTransfer(). The return value tells you if the device is functional and you can execute the transfer. Store it in tranferAllowed.
  • doReadTransferXxx()
    ** Only call _impl->readTransferXxx() if transferAllowed is true. If not return with hasNewData as false.
  • doPostRead()
    • Don't use the genericPostAction() any more.
    • Phase2 If (transferAllowed) DeviceModule::stopTransfer();
    • Try {_impl->postRead()} and catch the exception. If there was an exception
    • report it to the device module
    • set a local variable hasException to true
    • A decision depends on the phase
      • Phase 1 If (hasException or !transferAllowed)
      • Pase 2 If ((hasException or !transferAllowed) and previousReadFailed and (TransferType==read or TranferType==readAsync)) or (versionNumber == {nullptr})
    • If decision is true repeat the the following sequence until the read succeeds (does not thow)
      • waitForRecovery()
      • Phase 2 if (not DeviceModule::startTransfer()) go to beginning of loop
      • impl->read()
      • Phase 2 DeviceModule::stopTransfer()
        Also set "hasException" to false once the read succeeds.
    • setOwnerValidity(hasException)

Although in phase 1 everything could be done in the catch-block and setOwnerValitiy is always called with true, please implement it like this in preparation of phase 2.

When re-trying to read we always have to use a complete read cycle incl. preRead and postRead because we cannot repeat only the transfer. We are already behind the delegated postRead and have to call the full sequence.

Notice: Instead of transferAllowedyou cannot use TransferElement::readTransactionInProgress. If this is false the doPostRead is never called.

Sorry, I don't know if it's a good sign that the description of the implementation basically is the code. I always think good code should read like the newspaper.

Definition of done:

@killenb killenb changed the title Proper implementation of RecoveryAccessor::postRead() Proper implementation of RecoveryAccessor read operations Apr 15, 2020
@killenb
Copy link
Member Author

killenb commented Apr 16, 2020

Sorry, I don't know if it's a good sign that the description of the implementation basically is the code. I always think good code should read like the newspaper.

It obviously is a bad sign. I am micro-managing the tickets too much.

@killenb
Copy link
Member Author

killenb commented Jul 30, 2020

This ticket has become obsolete with the changes in the spec. The procedure described has changed. It is described in the spec, and implemented already

@killenb killenb closed this as completed Jul 30, 2020
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

No branches or pull requests

2 participants