Skip to content

Conversation

@ibwharri
Copy link
Collaborator

Changes experiments that use APT motors to have class based prefs. Updated emitter saturation experiment.

@ibwharri ibwharri requested a review from mwalsh161 February 25, 2020 22:58
Copy link
Collaborator

@ichristen ichristen left a comment

Choose a reason for hiding this comment

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

"Optionally also monitors a power meter to calibrate measurement with power."
This is important for a proper experiment, but doesn't seem to have been added back in.

Copy link
Owner

@mwalsh161 mwalsh161 left a comment

Choose a reason for hiding this comment

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

Have you run this yet?

end

function set.motor_serial_number(obj,val)
function val = set_motor_serial_number(obj,val,~)
Copy link
Owner

Choose a reason for hiding this comment

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

obj.motor_serial_number = val; is now unnecessary (below).

prefs = {'linein', 'lineout', 'acquire', 'nsamples', 'wavelength'};
prefs = {'angles','exposure','motor_move_time','motor_home_time','motor_serial_number'}; % String representation of desired prefs
% show_prefs = {}; % Use for ordering and/or selecting which prefs to show in GUI
%readonly_prefs = {}; % CC will leave these as disabled in GUI (if in prefs/show_prefs)
Copy link
Owner

Choose a reason for hiding this comment

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

line can be deleted

Comment on lines +10 to +11
APD_line = Prefs.String('APD1','help_text','NiDAQ line to apd','allow_empty',false);
APD_sync_line = Prefs.String('CounterSync','help_text','NiDAQ synchronisation line','allow_empty',false);
Copy link
Owner

Choose a reason for hiding this comment

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

Why not make these multiple choice? You can get the nidaq in/out lines from the device. Also, why don't you have to set the nidaq device anywhere O_O?

spacing = 2.25;
% Set methods allow validating property/pref set values
function newVal = setAngle(obj,val,pref)
obj.angle_list = str2num(val);
Copy link
Owner

Choose a reason for hiding this comment

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

str2num won't error on bad input. You should check for an empty array before assigning to angle_list.
Consider:

a = str2num('abc');

a will be [].

Comment on lines +77 to +79
if isnan(val_as_double)
return
end
Copy link
Owner

Choose a reason for hiding this comment

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

unreachable because of assert before.

% Setup graphics
y = NaN(1,Nangles);
hold(ax,'on');
plotH(1) = plot(obj.angle_list, y,'color', 'k','parent',ax);
Copy link
Owner

Choose a reason for hiding this comment

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

if only one plotH, why index into it? Also why specify color (not an issue, just curious)?

plotH(1) = plot(obj.angle_list, y,'color', 'k','parent',ax);
ylabel(ax,'Counts (cps)');
xlabel(ax,['Angle (' char(176) ')']);
yyaxis(ax, 'left');
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this being called? I only see one plot.

% Continuously acquires data from the APD and Thorlabs PM100 power meter and plots them.
% User should rotate the polarizer/HWP.

% Saturation changes intensity on a sample using a HWP (motorised or manually moved) and monitors the APD to measure saturation. Optionally also monitors a power meter to calibrate measurement with power.
Copy link
Owner

Choose a reason for hiding this comment

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

Does this really still support manually moving HWP?

Comment on lines +71 to +73
while (obj.rot.Moving || ~obj.rot.Homed) && (toc(t) < maxTime)
drawnow
if toc(t) > maxTime
Copy link
Owner

Choose a reason for hiding this comment

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

The chance that line 73 executes is extremely tiny (toc(t) would have had to go from < maxTime to > maxTime with only a drawnow between)! Your while loop is set to exit if maxTime is exceeded without erroring. This seems problematic?

Comment on lines +41 to +47
if ~isempty(obj.rot)
status.String = sprintf( 'Navigating to %g (%i/%i)', theta, ...
i, Nangles); drawnow;
obj.rot.move(theta);
else
pause(5)
end
Copy link
Owner

Choose a reason for hiding this comment

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

Assert above would prevent the else from ever happening. This seems to be the only thing that might give a user access to manually do it. But so many other things would break as they currently stand. I recommend just removing manual mode for now.

The easiest way to fix this would be to actually create a manual motor. Then, never change the code, but simply drop in a different motor that calls questdlg.

@ichristen
Copy link
Collaborator

Hey @ibwharri , what's the status of this pull request? Would be great to add this to CC after comments have been addressed!

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

Successfully merging this pull request may close these issues.

5 participants