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.

Sign up for GitHub

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

Jump to bottom

feat: ctmu capacitance measurement #104

Merged
merged 1 commit into from
Oct 28, 2021
Merged

feat: ctmu capacitance measurement #104

merged 1 commit into from
Oct 28, 2021

Conversation

CloudyPadmal
Copy link
Collaborator

@CloudyPadmal CloudyPadmal commented Jun 30, 2021 •
edited
Loading

Replaced #102

  • Fixed CTMU capacitance measurement function
  • Brought ADC1_Interrupt service routine back to adc1.c because it's being used by few other functions than oscilloscope.

@CloudyPadmal CloudyPadmal self-assigned this Jun 30, 2021
bessman
bessman reviewed Jun 30, 2021
View reviewed changes
pslab-core.X/instruments/multimeter.c Outdated
// Switch to high impedance state
CAP_OUT_SetDigitalInput();
CAP_OUT_SetAnalog();
Nop();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, what is the purpose of Nop here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This, I'm not sure. It's as it is from the original source. Need to test out the parts thoroughly and see if we can remove.

bessman
bessman reviewed Jun 30, 2021
View reviewed changes
pslab-core.X/instruments/multimeter.c Outdated
return SUCCESS;
}

response_t MULTIMETER_ReadCapacitance(void) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is quite large. It would benefit from being split up into more atomic units, I think.

bessman
bessman reviewed Jun 30, 2021
View reviewed changes
pslab-core.X/instruments/multimeter.c Outdated
Comment on lines 131 to 135
// TODO: Check the viability of the following block as it does nothing important
CAP_OUT_SetDigitalOutput();
CAP_OUT_SetDigital();
CAP_OUT_SetLow();
DELAY_us(50000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the purpose of this block is to ensure the capacitor is fully discharged before starting the measurement.

bessman
bessman reviewed Jun 30, 2021
View reviewed changes
pslab-core.X/registers/converters/adc1.c Outdated
@@ -138,7 +138,17 @@ void ADC1_SetOperationMode(
AD1CON3bits.ADCS = 0b00001010; // TAD = Tp*10 = 156.25 ns
break;
case ADC1_CTMU_MODE:
// initADCCTMU()
// Disable DMA channel
DMA0CONbits.CHEN = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use auto generated function DMA_ChannelDisable(DMA_CHANNEL_0) here.

bessman
bessman reviewed Jun 30, 2021
View reviewed changes
pslab-core.X/registers/converters/adc1.c Outdated
ADC1_ChannelSelectSet(current_channel_0);
// Clock settings
AD1CON3bits.SAMC = 0b10000; // 16*TAD auto sample time
AD1CON3bits.ADCS = 0b00001010; // TAD = Tp*10 = 156.25 ns
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we could use ADC1_ConversionClockPrescalerSet instead.

bessman
bessman previously approved these changes Jun 30, 2021
View reviewed changes
@CloudyPadmal
Copy link
Collaborator Author

Thanks alot for the review @bessman . I'll make the changes soon and push them here.

@CloudyPadmal CloudyPadmal marked this pull request as draft October 9, 2021 21:21
@CloudyPadmal CloudyPadmal marked this pull request as ready for review October 16, 2021 21:29
@CloudyPadmal
Copy link
Collaborator Author

Build fails due to Codacy analysis. It doesn't capture the fact that TRIGGER_CHANNEL and TRIGGER_READY params are set to something else than 0.

This was referenced Oct 17, 2021
Open
Open
bessman
bessman reviewed Oct 18, 2021
View reviewed changes
pslab-core.X/instruments/oscilloscope.c Outdated Show resolved Hide resolved
pslab-core.X/registers/converters/adc1.c Outdated
Comment on lines 249 to 255
TMR5_Stop();
T5CONbits.TSIDL = 1;
T5CONbits.TCKPS = 1;
TMR5 = 0;
TMR5_Start();
_T5IF = 0;
_T5IE = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed. InitModesCommon is called by Init10BitMode and InitDMAMode, which in turn are only called by the OSCILLOSCOPE module, which resets the timer after this point anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a mistake while rebasing the working branch. InitModesCommon shouldn't be touched by this PR. I'll update it.

pslab-core.X/registers/converters/adc1.h Outdated
Comment on lines 12 to 33

#define MAX_CHANNELS 4

/* Static globals */
static uint8_t volatile TRIGGERED = 0;
static uint8_t volatile TRIGGER_READY = 0;
static uint8_t TRIGGER_CHANNEL = 0;
static uint8_t CHANNELS = 0;
static uint8_t CONVERSION_DONE = 1;

static uint16_t DELAY;
static const uint16_t TRIGGER_TIMEOUT = 50000;
static uint16_t volatile TRIGGER_WAITING = 0;
static uint16_t TRIGGER_LEVEL = 0;
static uint16_t TRIGGER_PRESCALER = 0;

static int16_t volatile* volatile BUFFER_IDX[MAX_CHANNELS];
static uint16_t volatile* ADCVALS[MAX_CHANNELS] = {
&ADC1BUF0, &ADC1BUF1, &ADC1BUF2, &ADC1BUF3
};
static uint16_t volatile SAMPLES_CAPTURED;
static uint16_t SAMPLES_REQUESTED;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of this. Would be better if these were in adc.c and we added public getters and setters for them. I think that would also solve the Codacy complaint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Couldn't think of an efficient and "easy" way to sort out this variable set. Let's try getters and setters.

Copy link
Collaborator Author

@CloudyPadmal CloudyPadmal Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think the function names should be for getters and setters? Get_VAR() or GetVAR() or getVAR()? 🤔

pslab-core.X/instruments/multimeter.c Show resolved Hide resolved
@CloudyPadmal CloudyPadmal added firmware-porting Replacing older firmware with newer MCC based firmware and removed feature labels Oct 21, 2021
@CloudyPadmal CloudyPadmal merged commit 27f21ef into fossasia:bootloader Oct 28, 2021
@CloudyPadmal CloudyPadmal deleted the ctmu-capacitor-func branch October 28, 2021 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bessman bessman bessman approved these changes

Assignees

@CloudyPadmal CloudyPadmal

Labels
firmware-porting Replacing older firmware with newer MCC based firmware
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CloudyPadmal @bessman