Problem with Tiva Driverlib for ADC sequencer setup with channel over 0x0F

Hello James

Can you please put the code line which you are using?

Regards
Amit

See https://e2e.ti.com/support/microcontrollers/tiva_arm/f/908/t/435605

Are you sending the appropriate constants to ADCSequenceStepConfigure?  Note that ui32Config is NOT the channel number.

Robert

I did mention that uint32_t was a bad type choice for this.

Hello Robert

Yes, you did. Using enumerated data types was a better solution to avoid the same.

Regards
Amit

My code was taken from code written from TI. It is based on SMC SDK for TM4E123. While I know you dont support the TM4E anymore, I looked at the driverlib for tm4c and its the same. The codeset works fine for channels 0-15 but when it goes over 15 it does not set the channel mux and it accidentally sets the differential bit. My changes did resolve the problem

Hello James,

As Robert and myself mentioned the parameter that has been used is required to be correct. Now what was the value of the parameter above channel 15 that was used. If you can send that across it will be easier for us to analyze.

Regards
Amit

Actually looked more at the code and I see the rom is correct but because they dont define the constant for the channel in a linear manner. I actually am going to keep my code the way it is as it makes much more sense to have the channels in a linear fashion and is easier. Not sure why they did it the way they did as it doesnt make sense. The code base I am using from ti defines the gpios from a excel spreadsheet and runs a perl script to create a header file that loads all the gpios, pwms, i2c and analogs on its own. They treated the channels as linear which is why I was having the problems. 

James Dluhy said:

Not sure why they did it the way they did as it doesnt make sense

"Making sense" is a matter of perspective.

The TIVA library is a light interface layer over the H/W (That's its major strength IMO, a more heavy handed approach would limit its usefulness). As such H/W details show up in the API.

In my use I almost never need to refer to an A/D channel by an offset.  All of them are dedicated to a particular signal and the schedule is set up once and run forever, never changing. The channels are referred to by their signal rather than their position. As such they would normally be separate signal defines or enums in any case. The A/D setup would be done once either through a configuration table or straight code. In such a case using an index is of no particular help.

My most recent use is as a more general purpose I/O board.  Now in that case the signals do get referred to by channel.  However, the scheduling is still only done once and indexing would be of, at most, minimal help.  I think the straight line setup may actually be clearer and easier to read simply because of the way the scheduling works.

I can see that an indexed version might be useful in some cases. I'd do a wrapper rather than a replacement function though. That way you still retain the advantages of a tested library and your addition would be relatively light. You could also add some type safety to the wrapper^*.

Robert

* - cb1 has his hobby horse, I have might.  We'll ride away complaining of splinters

I guess I used my outside voice for that ;)

Robert

All I meant by making sense is that the code could have been written so that the channel number could be sent in order . There was no reason for the jump between 15 and 16 on the channel . For my app we have 67 gpios, 13 a2ds, 8 pwms, LPC, peci, and 5 i2cs, all initialized via an excel spreadsheet so that it can be changed rapidly for other motherboards. Indexing is very useful to us, and this system we worked from came straight from ti SDK in the first place. So losing a few hours on this enum instead of linear index to me was quite frustrating. But either way, all I need is a solution.

Robert Adsett said:

guess I used my outside voice

I'll say!   (as we're unable to hear, now...)

Again - Like ++ (your fire continues...)

Hello Robert

And the outside voice is equally well heard. Note that both of us started with the same observation and query on the use of the channel defines

Regards
Amit

I know James, that's why I mentioned that "making Sense" was a matter of perspective. The current structure actually does make sense to me for the way I use it.

BTW, I would suggest that any automatic code generation use the API define constants rather than indexes. If you must use indexes I would suggest using a wrapper rather than re-inventing the API, that's likely to lead to fewer future issues.

Robert

Since there have been a couple of confusions lately about the configure argument to ADCSequenceStepConfigureIndex, I thought I'd illustrate how to provide support for using channels numbers directly rather than the defines. If the process I'd add some type safety and other error detection and prevention and good code practice.

The code is incomplete, meant only as an illustration.  It may thus have typos, which I will try to correct if pointed out.  I do not provide all the needed includes, assuming that readers are sufficiently capable in C or C+++ to provide any missing includes.

So first the source for the function itself

/* Configures a step in an A/D sequence for a single ended A/D  conversion.  

   For illustrating 
        how to extend the existing library  while still taking advantage of the existing tested and ROM'd code.
        how to take advantage of types to improve error detection.
        how to use lint sematics for further increasing robustness.

   Not a complete function. Could be improved by adding support (or additional functions for)
        supporting differential mode
        supporting comparators
        more compile and run-time checks. */
void ADCSequenceStepConfigureIndex_SEAD(
	PERIPH_BASE_ADC base, 
	SEQ_ID sequence_num, 
	SEQ_STEP step, 
	AD_CHANNEL chnl, 
	bool last, 
	bool enable_interrupt)
{
     uint32_t config;

     EMB_ASSERT(((base == PB_ADC0_BASE) || (base == PB_ADC1_BASE)));
     EMB_ASSERT((sequence_num < MAX_ADSEQUENCES));
     EMB_ASSERT((step < MAX_ADSTEPS));
     EMB_ASSERT((chnl < MAX_ADCHANNELS));

	/* Could also be implemented as a lookup table. The code generated by the compiler will be similar.
	The guiding principles in choosing should be
		ease of understanding
		ease of maintenance
		ease of testing */
     switch( chnl) {
          case 0:
               config = ADC_CTL_CH0;
	       break;

          case 1:
               config = ADC_CTL_CH1;
	       break;

          case 2:
               config = ADC_CTL_CH2;
	       break;

          case 3:
               config = ADC_CTL_CH3;
	       break;

          case 4:
               config = ADC_CTL_CH4;
	       break;

          case 5:
               config = ADC_CTL_CH5;
	       break;

          case 6:
               config = ADC_CTL_CH6;
	       break;

          case 7:
               config = ADC_CTL_CH7;
	       break;

          case 8:
               config = ADC_CTL_CH8;
	       break;

          case 9:
               config = ADC_CTL_CH9;
	       break;

          case 10:
               config = ADC_CTL_CH10;
	       break;

          case 11:
               config = ADC_CTL_CH11;
	       break;

          case 12:
               config = ADC_CTL_CH12;
	       break;

          case 13:
               config = ADC_CTL_CH13;
	       break;

          case 14:
               config = ADC_CTL_CH14;
	       break;

          case 15:
               config = ADC_CTL_CH15;
	       break;

          case 16:
               config = ADC_CTL_CH16;
	       break;

          case 17:
               config = ADC_CTL_CH17;
	       break;

          case 18:
               config = ADC_CTL_CH18;
	       break;

          case 19:
               config = ADC_CTL_CH19;
	       break;

          case 20:
               config = ADC_CTL_CH10;
	       break;

		/* Need to do something safe in the event we have an invalid channel. */
          default:
               config = ADC_CTL_CH0;
	       break;

          } 

	/* Check for end of sequence and interrupt enable flags */
     if(last) {
          config |= ADC_CTL_END;
          }
     if(bool enable_interrupt) {
          config |= ADC_CTL_IE;
          }

	/* Now call the original configuration function */
     ADCSequenceStepConfigure((uint32_t)base, (uint32_t)sequence_num, (uint32_t)step, config);
}

And the accompanying header

	/* Currently allowed A/D converters, easily expanded at need */
typedef enum {
     PB_ADC0_BASE = ADC0_BASE,
     PB_ADC1_BASE = ADC1_BASE
     } PERIPH_BASE_ADC;

typedef uint_least8_t SEQ_ID; 		/* Allows up to 256 sequences, far more than current hardware supports */
typedef uint_least8_t SEQ_STEP; 	/* Allows up to 256 steps per sequence, far more than current hardware supports */
typedef uint_least8_t AD_CHANNEL; 	/* Allows up to 256 A/D channels, far more than current hardware supports */

#define MAX_ADSEQUENCES	((SEQ_ID)xxu)		/* Define as appropriate */
#define MAX_ADSTEPS	((SEQ_STEP)xxu)		/* Define as appropriate */
#define MAX_ADCHANNELS	((AD_CHANNEL)21u)	/* Define as appropriate, defined as 21 for illustration. */

/*lint -sem(ADCSequenceStepConfigureIndex_SEAD, (2n < MAX_ADSEQUENCES) && (3n < MAX_ADSTEPS) && (4n < MAX_ADCHANNELS)) */
/* Configures a step in an A/D sequence for a single ended A/D  conversion.  

   For illustrating 
        how to extend the existing library  while still taking advantage of the existing tested and ROM'd code.
        how to take advantage of types to improve error detection.
        how to use lint sematics for further increasing robustness.

   Not a complete function. Could be improved by adding support (or additional functions for)
        supporting differential mode
        supporting comparators
        more compile and run-time checks. */
void ADCSequenceStepConfigureIndex_SEAD(
	PERIPH_BASE_ADC base, 
	SEQ_ID sequence_num, 
	SEQ_STEP step, 
	AD_CHANNEL chnl, 
	bool last, 
	bool enable_interrupt);

So what should be noted?

• First all the arguments have proper unique types suited to their purpose. This allows mismatches to be detected at compile time. C and C++ users can detect this via lint and C++ will also provide some protection on its own. Note that the type for the A/D base address is restricted to valid A/D addresses only. It should not be possible to use a random number or the value of another peripheral.
• The sequence number, step number and channel are all unique types to prevent using  step number to select a channel. Such simple exchanges of arguments are otherwise very difficult to track down.
• Last entry and interrupt enable have been separated from the channel index.
• Runtime checks have been added for range checking. These are of minimal use due to their intrusive nature but can be helpful during initial testing and are potentially useful for TDD.
• Only the A/D channel has been properly dealt with in case of an invalid argument range.
• This should have it's own set of TDD style test cases but there is a limit to how much is useful in an illustration.
• Note the lint -sem( comment. This is a lint extension (it's done in a fashion that makes it transparent to compilers etc...) that allows lint to check arguments. In this case lint is being given a set of inequalities that range check the second, third and fourth arguments. This allows lint to produce an error or warning if the argument passed is (or is likely to be) out of range. This is in addition to the type checks.
• This maintains the power of the existing code
• Finally I have removed the ugly and misleading hungarian notation.  A code blight if ever there was one.

Robert

Bravo Robert - superbly detailed, very nicely organized, and explained w/great care!   (although my famed HP16C recoils at being labeled (both) ugly & misleading!)

May I suggest that you consider the addition of (some) "marker" to indicate that an invalid channel has been detected - and that, "ADC_CTL_CH0" has (instead) been substituted?

BTW - 3 more coats of shellac have rendered (my) hobby horse, "splinter free."    (although mounting - & remaining on at speed - (now) prove an adventure...)

Yeah, I left out most of the range error checking in the interests of brevity and laziness.

Certainly an error return makes sense. Probably in that case it would make sense to not perform any setup at all. In some cases I can see that the best response to an error might be to try to stumble on, in others it might be to reset and retry. An error return gives great flexibility in a general purpose API.

Robert

think HP16C fails as Hungarian notation. And RPN was never properly appreciated.

Are you sure that's enough shellac? There are some pretty big splinters.

Robert

Robert Adsett72 said:

Are you sure that's enough shellac?

In school we were taught, "never to "be sure" and/or never to say, "never!"

And - lest we forget - while the rear-ends of some forum members have expanded - my 308's rear view mirror reports, "Objects may appear larger than normal."   (thus - "optical illusion" is my defense - and I'm sticking with it...)   And really - did my gleaming, tri-poly coated horse - just groan?