Skip to content

Added nullptr check in EmotibitEda_cpp#296

Merged
nitin710 merged 8 commits intofeat-FileTransferfrom
fix-emotibitEda
Mar 25, 2024
Merged

Added nullptr check in EmotibitEda_cpp#296
nitin710 merged 8 commits intofeat-FileTransferfrom
fix-emotibitEda

Conversation

@nitin710
Copy link
Collaborator

@nitin710 nitin710 commented Feb 14, 2024 •
edited
Loading

Description

  • Added nullptr checks. These were causing a crash on the agave board.

Requirements

  • None

Issues Referenced

Documentation update

  • None

Testing

  • This is a minor patch, so there is no test added in the Feature test protocol doc.
    • In the future, when we do implement a mechanism of "selectively setting up sensors", we will probably add a test to check that the fw does not crash if using a sensors module not setup
  • For now, i just test it by bypassing eda setup in the setup()
 // Setup EDA
	#if(0) // #if defined out
	{
	Serial.println("\nInitializing EDA... ");
	if (emotibitEda.setup(_hwVersion, _samplingRates.eda / ((float)_samplesAveraged.eda), &eda, &edl, &edr, _EmotiBit_i2c, &edlBuffer, &edrBuffer))
	{
		EmotiBitFactoryTest::updateOutputString(factoryTestSerialOutput, EmotiBitFactoryTest::TypeTag::ADC_INIT, EmotiBitFactoryTest::TypeTag::TEST_PASS);
		Serial.println("Completed");
	}
	else
	{
		EmotiBitFactoryTest::updateOutputString(factoryTestSerialOutput, EmotiBitFactoryTest::TypeTag::ADC_INIT, EmotiBitFactoryTest::TypeTag::TEST_FAIL);
		Serial.println(factoryTestSerialOutput);
		Serial.println("failed");
	}
	}
	Serial.println("\nLoading EDA calibration... ");
	if (emotibitEda.stageCalibLoad(&_emotibitNvmController, true))
	{
		Serial.println("Completed");
	}
	else
	{
		Serial.println("failed");
	}
	#endif

and the oscilloscope continued set up and continued normal function + recording without eda (which was previously crashing for the agave board)
image

produceconsumerobot
produceconsumerobot requested changes Mar 13, 2024
View reviewed changes
Copy link
Collaborator

@produceconsumerobot produceconsumerobot left a comment

Choose a reason for hiding this comment

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

@nitin710 review complete

EmotiBitEda.cpp Outdated

if (_emotibitVersion >= EmotiBitVersionController::EmotiBitVersion::V04A)
{
if(_edlOversampBuffer == nullptr || _edlBuffer == nullptr) return 16; // BufferFloat::ERROR_PTR_NULL = 16. But, any non-zero value sohuld work.
Copy link
Collaborator

@produceconsumerobot produceconsumerobot Mar 13, 2024

Choose a reason for hiding this comment

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

Is there a reason we wouldn't return (uint8_t) BufferFloat::ERROR_PTR_NULL to avoid magic numbers?

Copy link
Collaborator Author

@nitin710 nitin710 Mar 21, 2024

Choose a reason for hiding this comment

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

I thought it needs a EmotiBitEda::NULL_PTR_ERROR. So i just added a number, with a comment explaining it.
But since, the buffers are BufferFloat under the hood, we could use BufferFloat::NULL_PTR anyway.

Changing it to (uint8_t) BufferFloat::ERROR_PTR_NULL

EmotiBitEda.cpp Outdated
{
// Reads EDA data from ADC

if(_edlOversampBuffer == nullptr || _edrOversampBuffer == nullptr || _edlBuffer == nullptr || _edrBuffer == nullptr) return 16; // BufferFloat::ERROR_PTR_NULL = 16. But, any non-zero value sohuld work.
Copy link
Collaborator

@produceconsumerobot produceconsumerobot Mar 13, 2024

Choose a reason for hiding this comment

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

Is there a reason we wouldn't return (uint8_t) BufferFloat::ERROR_PTR_NULL to avoid magic numbers?

Copy link
Collaborator Author

@nitin710 nitin710 Mar 21, 2024

Choose a reason for hiding this comment

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

See https://github.com/EmotiBit/EmotiBit_FeatherWing/pull/296/files/b403652a673837a8b6b99d02a22d6746c710e332#r1533223423

EmotiBitEda.cpp Outdated
}
else
{
if(_edlBuffer == nullptr || _edrBuffer == nullptr || _edaBuffer == nullptr) return false;
Copy link
Collaborator

@produceconsumerobot produceconsumerobot Mar 13, 2024

Choose a reason for hiding this comment

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

Does _edrOversampBuffer need checking here too?

Copy link
Collaborator Author

@nitin710 nitin710 Mar 21, 2024

Choose a reason for hiding this comment

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

Added this check in the OR statement.

@nitin710
Copy link
Collaborator Author

nitin710 commented Mar 21, 2024

Changes made, as requested.

@produceconsumerobot
Copy link
Collaborator

produceconsumerobot commented Mar 23, 2024

@nitin710 Looks good

@nitin710 nitin710 merged commit 282ef95 into feat-FileTransfer Mar 25, 2024
@nitin710 nitin710 deleted the fix-emotibitEda branch March 25, 2024 22:14
@nitin710 nitin710 mentioned this pull request Mar 28, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@produceconsumerobot produceconsumerobot produceconsumerobot requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nitin710 @produceconsumerobot