ForumSections

iCustom giving unexpected values - page 3

New comment
[Deleted]
Tristen Shaw #: This is my new code:

Ok! Now lets tackle the OnInit. I will ignore the general structure and parameter inputs for now, but will come back to it later. I am currently focused on routing out problem areas. I am not trying to find your "bug" yet. My aim is to systematically improve the logic and remove future problems. The cleaner the code, the easier it will be to spot the actual bug. So ...

Currently in your OnInit code, you are doing the following in this sequence:

  1. Assign buffers
  2. Define plot settings
  3. Obtain handles to other indicators
  4. Test parameters
  5. Test handles

Don't you think that something is wrong with that sequence?

Given that some of the operations will invalidate and force the indicator to terminate, don't you think you should follow a different sequence?

  1. Test parameters 
  2. Obtain handles to other indicators 
  3. Test handles
  4. Assign buffers
  5. Define plot settings
Fernando Carreiro #:

Ok! Now lets tackle the OnInit. I will ignore the general structure and parameter inputs for now, but will come back to it later. I am currently focused on routing out problem areas. I am not trying to find your "bug" yet. My aim is to systematically improve the logic and remove future problems. The cleaner the code, the easier it will be to spot the actual bug. So ...

Currently in your OnInit code, you are doing the following in this sequence:

  1. Assign buffers
  2. Define plot settings
  3. Obtain handles to other indicators
  4. Test parameters
  5. Test handles

Don't you think that something is wrong with that sequence?

Given that some of the operations will invalidate and force the indicator to terminate, don't you think you should follow a different sequence?

  1. Test parameters 
  2. Obtain handles to other indicators 
  3. Test handles
  4. Assign buffers
  5. Define plot settings

    Ok, I see. Now, is this mainly a matter of efficiency? Like "Why have the computer do all this other work if it's possible that the tests might terminate the program anyway. Or is there a further reason aside from that?

    New OnInit:

    int OnInit(){
//--- indicator buffers mapping
   if( !( atrCrossLevel > 0 ) ) {
         Print( "Error: Invalid ATR Cross Level" );
         return INIT_PARAMETERS_INCORRECT;
   };
   
   atrHandle = iATR(Symbol(), Period(),
                    atrPeriod);
                    
   rexHandle = iCustom(Symbol(), Period(), "REX",
                       confirmPeriod,
                       confirmPeriodMethod,
                       confirmSignal,
                       confirmSignalMethod);
                       
   if(rexHandle == INVALID_HANDLE || atrHandle == INVALID_HANDLE){
      //--- tell about the failure and output the error code
      PrintFormat( "Failed to create handle of the iCustom indicator for the symbol %s/%s, error code %d",
                  Symbol(),
                  EnumToString(Period()),
                  GetLastError());
      //--- the indicator is stopped early
      initError = true;
      return(INIT_FAILED);
   }
   
   SetIndexBuffer(0,  sellBuffer,       INDICATOR_DATA);
   SetIndexBuffer(1,  buyBuffer,        INDICATOR_DATA);
   SetIndexBuffer(2,  atrBuffer,        INDICATOR_CALCULATIONS);
   SetIndexBuffer(3,  rexPeriodBuffer,  INDICATOR_CALCULATIONS);
   SetIndexBuffer(4,  rexSignalBuffer,  INDICATOR_CALCULATIONS);
   
   PlotIndexSetDouble(0, PLOT_EMPTY_VALUE, 0.0);
   PlotIndexSetDouble(1, PLOT_EMPTY_VALUE, 0.0);
   
   PlotIndexSetInteger(0, PLOT_ARROW, 159);
   PlotIndexSetInteger(1, PLOT_ARROW, 159);                                 
   
//---
   return(INIT_SUCCEEDED);
}
    [Deleted]
    Tristen Shaw #: Ok, I see. Now, is this mainly a matter of efficiency? Like "Why have the computer do all this other work if it's possible that the tests might terminate the program anyway. Or is there a further reason aside from that? New OnInit:

    Yes, and no! Yes, it is mainly the reason, but there is a second part which I am going to address now.

    What do you think happens to the Indicator handles that you obtain?

    Surely they need to be released when you no longer need them. One would assume that the terminal takes care of them when you terminate the indicator, but what if it does not?

    Then we better make sure that our code does a proper clean-up. And we have to do it in two places. So, the sequence we follow in OnInit can be quite important.

    One part, which you have not implemented, is during the OnDeinit() event handler and the other, is when you test the handles, when maybe one of them is invalid and you exit the OnInit() with a failed attempt.

    To release the handles, you have to use IndicatorRelease. Have a look at the example code there. Also have a look at the example code for iATR, for the OnDeinit section and then also use the logic in your "testing handles" section.

    This time I will wait for your attempt before I give you mine.

    Fernando Carreiro #:

    Yes, and no! Yes, it is mainly the reason, but there is a second part which I am going to address now.

    What do you think happens to the Indicator handles that you obtain?

    Surely they need to be released when you no longer need them. One would assume that the terminal takes care of them when you terminate the indicator, but what if it does not?

    Then we better make sure that our code does a proper clean-up. And we have to do it in two places. So, the sequence we follow in OnInit can be quite important.

    One part, which you have not implemented, is during the OnDeinit() event handler and the other, is when you test the handles, when maybe one of them is invalid and you exit the OnInit() with a failed attempt.

    To release the handles, you have to use IndicatorRelease. Have a look at the example code there. Also have a look at the example code for iATR, for the OnDeinit section and then also use the logic in your "testing handles" section.

    This time I will wait for your attempt before I give you mine.

    Alright, here is what I came up with.

    int OnInit(){
//--- indicator buffers mapping
   if( !( atrCrossLevel > 0 ) ) {
      initError = true;
      Print( "Error: Invalid ATR Cross Level" );
      return INIT_PARAMETERS_INCORRECT;
   };
   
   atrHandle = iATR(Symbol(), Period(),
                    atrPeriod);
                    
   rexHandle = iCustom(Symbol(), Period(), "REX",
                       confirmPeriod,
                       confirmPeriodMethod,
                       confirmSignal,
                       confirmSignalMethod);
                       
   if(rexHandle == INVALID_HANDLE || atrHandle == INVALID_HANDLE){
      //--- tell about the failure and output the error code
      PrintFormat( "Failed to create handle of the iCustom indicator for the symbol %s/%s, error code %d",
                  Symbol(),
                  EnumToString(Period()),
                  GetLastError());
      //--- the indicator is stopped early
               
      initError = true;
      return(INIT_FAILED);
   }
   
   if(initError){
      if(!IndicatorRelease(atrHandle))
      Print("IndicatorRelease() failed. Error ",GetLastError());
      
      if(!IndicatorRelease(rexHandle))
         Print("IndicatorRelease() failed. Error ",GetLastError());
   }
//. . . other code



void OnDeinit(const int reason){
   if(!IndicatorRelease(atrHandle))
      Print("IndicatorRelease() failed. Error ",GetLastError());
      
   if(!IndicatorRelease(rexHandle))
      Print("IndicatorRelease() failed. Error ",GetLastError());
}      
    [Deleted]
    Tristen Shaw #: Alright, here is what I came up with.

    And here is my rendition ...

    // Declare indicator handles
   int
      atrHandle = INVALID_HANDLE,   // Handle for ATR indicator
      rexHandle = INVALID_HANDLE;   // Handle for REX indicator

// Support function to release a single indicator handle
   void ReleaseHandle( int &hHandle, string sName  )
   {
      if( hHandle != INVALID_HANDLE )
      {
         ResetLastError();
         if( IndicatorRelease( hHandle ) )
            hHandle = INVALID_HANDLE;
         else
            PrintFormat( "Error %d: Unable to release %s indicator handle", _LastError, sName );
      };
   };

// Support function to release all indicator handles
   void ReleaseAllHandles( void )
   {
      ReleaseHandle( atrHandle, "iATR" );
      ReleaseHandle( rexHandle, "REX"  );
   };

// De-initialisation event handler
   void OnDeinit( const int reason )
   { ReleaseAllHandles(); };
   
// Initialisation event handler
   int OnInit()
   {
      // Check parameters
         if( !( atrCrossLevel > 0 ) )
         {
            Print( "Error: Invalid ATR Cross Level" );
            return INIT_PARAMETERS_INCORRECT;
         };
   
      // Obtain indicator handles
         ResetLastError();
         atrHandle = iATR(    _Symbol, _Period, atrPeriod );
         rexHandle = iCustom( _Symbol, _Period, "REX",
                       confirmPeriod, confirmPeriodMethod, confirmSignal, confirmSignalMethod );
                       
      // Check validity of handles
         if( atrHandle == INVALID_HANDLE || rexHandle == INVALID_HANDLE )
         {
            PrintFormat( "Error %d: Failed to obtain indicator handles for symbol: %s, time-frame: %s",
               _LastError, _Symbol, EnumToString( _Period ) );
            ReleaseAllHandles();
            return INIT_FAILED;
         };
         
      // other code ...

      return INIT_SUCCEEDED;  // Successful initialisation of indicator
   };

    What do you see that is different?

    Do you understand the differences?

    Do you agree with the differences?

    Remember that I can make mistakes too!

    [Deleted]
    I'm off to sleep now, so I will pick this up and continue tomorrow.
    Fernando Carreiro #:

    And here is my rendition ...

    What do you see that is different?

    Do you understand the differences?

    Do you agree with the differences?

    Remember that I can make mistakes too!

    Well, the first thing I notice that you didn't use the IndicatorRelease function at all in your code. I have to assume that there is a reason for this, but I can't say whether I agree or disagree with it since I don't know if there is some kind of advantage to doing it this way or not.

    Otherwise, the function seems good. Easily expandable if new indicators where to be introduced later. Prevents the code from getting cluttered with lines and lines of the same error handling and such, replacing it with a simple function call.

    I guess I would have to say that I agree with the differences. It's cleaner and more organized.

    [Deleted]

    Tristen Shaw #: Well, the first thing I notice that you didn't use the IndicatorRelease function at all in your code.

    Look again ... OnDeinit() calls ReleaseAllHandles(), which calls ReleaseHandle() which calls IndicatorRelease().

    [Deleted]
    Ok, now I'm really off to sleep and tomorrow we will tackle the OnCalculate() and checking that the buffers are being filled correctly by CopyBuffer().
    [Deleted]

    Ok, here is continuing our saga ...

    Next, some test code to simply copy and display the data obtained from iATR. Put up this indicator and the standard ATR indicator so as to compare the two.

    Then look at the code and analyse, question and comment on it.

    // Define indicator properties

   // Define plotting conditions
      #property indicator_separate_window
      #property indicator_buffers 1
      #property indicator_plots   1

   // Define properties for ATR plot
      #property indicator_label1  "ATR"
      #property indicator_type1   DRAW_LINE
      #property indicator_color1  clrGreen
      #property indicator_style1  STYLE_SOLID
      #property indicator_width1  1

// Declare input variables
   input uint i_nAtrPeriod = 14;       // Period for average true range
   
// Declare buffer arrays
   double g_adbAtrBuffer[];            // Array for ATR buffer
   
// Declare indicator handles
   int g_hAtrHandle = INVALID_HANDLE;  // Handle for ATR indicator

// Support function to release indicator handle
   void ReleaseHandle( int &hHandle, string sName )
   {
      if( hHandle != INVALID_HANDLE )
      {
         ResetLastError();
         if( IndicatorRelease( hHandle ) )
            hHandle = INVALID_HANDLE;
         else
            Print( "Error %d: Unable to release %s indicator handle", _LastError, sName );
      };
   };

// Support function to release all indicator handles
   void ReleaseAllHandles( void )
   {
      ReleaseHandle( g_hAtrHandle, "iATR" );
   };

// De-initialisation event handler
   void OnDeinit( const int reason )
   { ReleaseAllHandles(); };
   
// Initialisation event handler
   int OnInit()
   {
      // Check parameters
         if( i_nAtrPeriod < 1 )
         {
            Print( "Error: Invalid ATR period" );
            return INIT_PARAMETERS_INCORRECT;
         };
   
      // Obtain indicator handles
         ResetLastError();
         g_hAtrHandle = iATR( _Symbol, _Period, i_nAtrPeriod );
                       
      // Check validity of handls
         if( g_hAtrHandle == INVALID_HANDLE )
         {
            PrintFormat( "Error %d: Failed to obtain indicator handles for symbol: %s, time-frame: %s",
               _LastError, _Symbol, EnumToString( _Period ) );
            ReleaseAllHandles();
            return INIT_FAILED;
         };
         
      // Set buffers
         SetIndexBuffer( 0, g_adbAtrBuffer, INDICATOR_DATA );

      return INIT_SUCCEEDED;  // Successful initialisation of indicator
   };

// OnCalculate event handler
   int OnCalculate( const int rates_total,
                    const int prev_calculated,
                    const datetime &time[],
                    const double &open[],
                    const double &high[],
                    const double &low[],
                    const double &close[],
                    const long &tick_volume[],
                    const long &volume[],
                    const int &spread[] )
   {
      // Make sure we have rates data
         if( rates_total < 1 ) return 0; // No data available yet
         
      // Obtain and check number of calculated bars for ATR indicator
         ResetLastError();
         int nAtrBarsCalc = BarsCalculated( g_hAtrHandle );
         if( nAtrBarsCalc < rates_total )
         {
            PrintFormat( "Error %d: Insufficient ATR indicator data available", _LastError );
            return 0;
         };

      // Calculate bar counting variables
         int
            // Adjust number of previously calculated bars accounting for incomplete current bar
               nPrevCalc    =   prev_calculated > rates_total ? rates_total     - 1
                            : ( prev_calculated > 1           ? prev_calculated - 1 : 0 ),
            // Calculate how many bars need to be updated
               nBarsUpdate  = rates_total - nPrevCalc;
         
      // Collect ATR data
         ResetLastError();
         int nAtrBarsCopied = CopyBuffer( g_hAtrHandle, 0, 0, nBarsUpdate, g_adbAtrBuffer );
         if( nAtrBarsCopied < nBarsUpdate )
         {
            PrintFormat( "Error %d: Insufficient ATR data copied", _LastError );
            return 0;
         };

      return rates_total;  // Return value for prev_calculated of next call
   };

    Don't worry. We will slowly build it up again to the full code you are developing.

    12345678
    New comment